feat: Enhance cortex ask with interactive tutor capabilities
#395
Conversation
…ortexlinux#393) - Enhanced system prompt to detect educational vs diagnostic queries - Added LearningTracker class for tracking educational topics - Learning history stored in ~/.cortex/learning_history.json - Increased max_tokens from 500 to 2000 for longer responses - Added terminal-friendly formatting rules - Rich Markdown rendering for proper terminal display - Added 25 new unit tests (50 total) for ask module - Updated COMMANDS.md with cortex ask documentation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughAdds a LearningTracker to detect, extract, and persist educational topics to Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AskHandler
participant LearningTracker
participant FileSystem as File System
participant LLM as OpenAI/Claude
User->>AskHandler: ask(question)
AskHandler->>LearningTracker: is_educational_query(question)
LearningTracker-->>AskHandler: true/false
alt Educational
AskHandler->>LearningTracker: extract_topic(question)
LearningTracker-->>AskHandler: topic
AskHandler->>LearningTracker: record_topic(question)
LearningTracker->>FileSystem: _load_history()
FileSystem-->>LearningTracker: history
LearningTracker->>FileSystem: _save_history(updated_history)
end
AskHandler->>LLM: call with expanded system prompt + max_tokens=2000
LLM-->>AskHandler: response
AskHandler-->>User: return formatted Markdown response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 0
🧹 Nitpick comments (2)
cortex/ask.py (2)
172-200: Consider edge case in topic truncation logic.Line 199 truncates topics to 50 characters at word boundaries using
topic[:50].rsplit(" ", 1)[0]. If the first 50 characters contain no spaces,rsplitreturns a single-element list, and[0]will return the full 50-character string, which is correct.However, this could still result in awkwardly truncated topics for long compound words or URLs. Consider whether 50 characters is sufficient for your use cases.
Example edge case:
# Topic with no spaces in first 50 chars topic = "verylongcompoundwordwithoutanyspacesthatexceeds50characters" # Result: "verylongcompoundwordwithoutanyspacesthatexceeds50"The current behavior is safe but might not be ideal for all inputs.
255-262: Silent failure on save may complicate debugging.Line 262 silently suppresses OSError when writing the learning history file. While this prevents the CLI from crashing when the history file can't be written (e.g., permission issues, disk full), it makes debugging difficult if users expect their learning history to be tracked but it's failing silently.
Consider whether this trade-off aligns with your UX goals. For a "Chill" review mode, this is acceptable, but you might want to log these failures in verbose mode in the future.
Based on learnings from similar patterns in the codebase.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cortex/ask.pycortex/cli.pydocs/COMMANDS.mdtests/test_ask.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/cli.pytests/test_ask.pycortex/ask.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_ask.py
🧬 Code graph analysis (1)
tests/test_ask.py (1)
cortex/ask.py (11)
ask(458-526)LearningTracker(139-262)SystemInfoGatherer(20-136)is_educational_query(165-170)extract_topic(172-200)record_topic(202-225)get_history(227-229)get_recent_topics(231-242)get_recent_topics(536-545)gather_context(129-136)_get_system_prompt(334-391)
⏰ 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: Test (Python 3.10)
- GitHub Check: test (3.12)
- GitHub Check: test (3.11)
- GitHub Check: test (3.10)
🔇 Additional comments (13)
cortex/cli.py (2)
9-10: LGTM! Appropriate import for Markdown rendering.The import extends the existing Rich library usage to support formatted terminal output for the enhanced educational responses.
302-303: LGTM! Clean integration of Markdown rendering.The change properly leverages Rich's Markdown class to render formatted educational responses in the terminal, aligning with the enhanced output formatting rules in the system prompt.
tests/test_ask.py (3)
273-396: Excellent test coverage for LearningTracker.The test class comprehensively covers:
- Educational query pattern detection across multiple patterns
- Topic extraction with various prefixes
- Truncation handling for long topics
- History persistence and incremental updates
- Non-educational query filtering
The use of temporary files and proper patching ensures test isolation.
398-460: LGTM! Strong integration tests for learning features.The tests validate:
- Educational queries are properly recorded through AskHandler
- Diagnostic queries are correctly excluded from learning history
- Recent topics retrieval works through the handler API
- System prompt contains the necessary educational guidance
Good use of the fake provider to avoid external API dependencies.
462-493: Good validation of system prompt enhancements.The tests ensure the enhanced system prompt includes:
- Query type detection guidance
- Educational response instructions with examples and best practices
- Diagnostic response instructions with system context
This validates the PR's core objective of distinguishing between query types.
docs/COMMANDS.md (2)
75-132: Excellent documentation for the enhanced ask command.The documentation clearly explains:
- The dual nature of the command (diagnostic vs educational)
- Automatic intent detection
- System-aware responses and learning progress tracking
- Practical examples for both query types
The feature list and notes provide users with a complete understanding of the command's capabilities.
429-445: Well-structured learning workflow section.The workflow provides a clear progression:
- Diagnostic queries for system information
- Educational queries for learning
- Step-by-step tutorials
- Automatic tracking of learning history
This practical guide helps users understand and leverage the new capabilities.
cortex/ask.py (6)
1-17: LGTM! Clean import additions and updated docstring.The new imports (re, datetime, Path) are from the standard library and support the learning tracking functionality. The docstring accurately reflects the expanded scope to include educational content and progress tracking.
139-170: Well-designed LearningTracker class with comprehensive patterns.The class structure is solid:
- Pre-compiled regex patterns for efficiency
- Comprehensive educational query detection patterns
- Case-insensitive matching appropriately handles user input variations
The pattern list covers the main educational query types mentioned in the PR objectives.
288-288: LGTM! Proper integration of LearningTracker.The learning tracker is correctly instantiated in
__init__, ensuring it's available for all ask operations.
334-391: Excellent system prompt design for dual-mode operation.The enhanced system prompt effectively:
- Distinguishes between educational and diagnostic query types with clear trigger patterns
- Provides specific guidelines for each response type
- Includes explicit output formatting rules to prevent terminal rendering issues
- Provides a concrete example to guide the LLM
The level of detail is appropriate for ensuring consistent, high-quality responses across both query modes. The terminal-specific formatting rules (avoiding markdown headings, limiting line length) are particularly valuable.
401-401: Appropriate token increase for educational responses.Increasing max_tokens from 500 to 2000 for both OpenAI and Claude is necessary to support the longer, structured educational responses with code examples and best practices. The 4x increase aligns with the PR's objective to provide comprehensive tutorials.
Note: This will increase API costs, but is appropriate for the enhanced functionality.
Also applies to: 413-413
523-545: Clean integration and well-designed public API.The learning tracking integration:
- Records topics at the appropriate point (after successful answer generation)
- Exposes a clean public API through
get_learning_history()andget_recent_topics()- Follows Python conventions with proper type hints and docstrings
- Delegates appropriately to the LearningTracker instance
The API design allows external code to access learning history without tight coupling to the tracker implementation.
There was a problem hiding this comment.
@ShreeJejurikar Kindly Update your branch and merge with main. and follow contribution guidelines.
CLA Verification PassedAll contributors have signed the CLA.
|
There was a problem hiding this comment.
Pull request overview
This PR enhances the cortex ask command to automatically detect and respond appropriately to both educational and diagnostic queries, eliminating the need for a separate cortex tutor command. The system intelligently distinguishes between learning-focused questions (e.g., "explain how Docker works") and system-specific diagnostics (e.g., "why is my disk full"), providing structured tutorials with code examples for the former and actionable system information for the latter.
Key changes:
- Added
LearningTrackerclass to detect educational queries using regex patterns and persist learning history to~/.cortex/learning_history.json - Enhanced system prompt with query type detection instructions and terminal-friendly formatting guidelines
- Increased
max_tokensfrom 500 to 2000 to accommodate longer educational responses - Added 25 new unit tests for learning tracker functionality
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
cortex/ask.py |
Implemented LearningTracker class for educational query detection and history tracking; enhanced system prompt with dual-mode instructions; increased token limits |
cortex/cli.py |
Added Rich Markdown rendering for properly formatted terminal output |
tests/test_ask.py |
Added comprehensive test coverage for LearningTracker class and enhanced system prompt validation |
docs/COMMANDS.md |
Added cortex ask command documentation with examples of diagnostic and educational queries; included learning history feature description |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
docs/COMMANDS.md
Outdated
|
|
||
| **Notes:** | ||
| - Educational responses are longer and include code examples with syntax highlighting | ||
| - The `--offline` flag can be used to only return cached responses |
There was a problem hiding this comment.
The documentation mentions an --offline flag, but this flag is not actually implemented in the CLI argument parser for the ask command. The ask command only accepts a question argument and has no optional flags defined. Either implement the --offline flag functionality or remove this reference from the documentation.
| - The `--offline` flag can be used to only return cached responses |
cortex/ask.py
Outdated
| if len(topic) > 50: | ||
| topic = topic[:50].rsplit(" ", 1)[0] |
There was a problem hiding this comment.
There's an edge case bug in the topic truncation logic. When a topic is exactly 50 characters long, calling rsplit(" ", 1) will split it and return only the first part, potentially removing the last word unnecessarily. The condition should check if the topic is greater than 50 characters before attempting truncation, or the rsplit should only be applied after confirming there's content beyond 50 characters.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| def test_is_educational_query_best_practices(self): | ||
| """Test detection of 'best practices' queries.""" | ||
| self.assertTrue(self.tracker.is_educational_query("best practices for security")) | ||
| self.assertTrue(self.tracker.is_educational_query("what are best practice for nginx")) |
There was a problem hiding this comment.
Spelling error: "practice" should be "practices" to match the plural form used elsewhere in the documentation and code.
| self.assertTrue(self.tracker.is_educational_query("what are best practice for nginx")) | |
| self.assertTrue(self.tracker.is_educational_query("what are best practices for nginx")) |
cortex/ask.py
Outdated
| ## Query Type Detection | ||
| Automatically detect the type of question and respond appropriately: | ||
| ### Educational Questions (tutorials, explanations, learning) | ||
| Triggered by questions like: "explain...", "teach me...", "how does X work", "what is...", "best practices for...", "tutorial on...", "learn about...", "guide to..." | ||
| For educational questions: | ||
| 1. Provide structured, tutorial-style explanations | ||
| 2. Include practical code examples with proper formatting | ||
| 3. Highlight best practices and common pitfalls to avoid | ||
| 4. Break complex topics into digestible sections | ||
| 5. Use clear section labels and bullet points for readability | ||
| 6. Mention related topics the user might want to explore next | ||
| 7. Tailor examples to the user's system when relevant (e.g., use apt for Debian-based systems) | ||
| ### Diagnostic Questions (system-specific, troubleshooting) | ||
| Triggered by questions about: current system state, "why is my...", "what packages...", "check my...", specific errors, system status | ||
| For diagnostic questions: | ||
| 1. Analyze the provided system context | ||
| 2. Give specific, actionable answers | ||
| 3. Be concise but informative | ||
| 4. If you don't have enough information, say so clearly | ||
| 5. For package compatibility questions, consider the system's Python version and OS | ||
| 6. Return ONLY the answer text, no JSON or markdown formatting""" | ||
| ## Output Formatting Rules (CRITICAL - Follow exactly) |
There was a problem hiding this comment.
The system prompt contains markdown headings (## Query Type Detection, ### Educational Questions, ### Diagnostic Questions, ## Output Formatting Rules) despite instructing the LLM in the "Output Formatting Rules" section to "NEVER use markdown headings (# or ##) - they render poorly in terminals". While these headings are for organizing the system prompt itself (not the LLM's output), this inconsistency could confuse the model or make it less likely to follow the formatting rules. Consider using bold text for these section titles in the system prompt as well, to demonstrate the desired formatting style.
tests/test_ask.py
Outdated
| @@ -1,14 +1,16 @@ | |||
| """Unit tests for the ask module.""" | |||
|
|
|||
| import json | |||
There was a problem hiding this comment.
Import of 'json' is not used.
| import json |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
cortex/ask.py (3)
139-163: Consider making compiled patterns a class variable.The
_compiled_patternslist is recompiled for everyLearningTrackerinstance, but sinceEDUCATIONAL_PATTERNSis static, the compiled patterns could be shared across all instances.🔎 Proposed refactor
class LearningTracker: """Tracks educational topics the user has explored.""" PROGRESS_FILE = Path.home() / ".cortex" / "learning_history.json" # Patterns that indicate educational questions EDUCATIONAL_PATTERNS = [ r"^explain\b", r"^teach\s+me\b", r"^what\s+is\b", r"^what\s+are\b", r"^how\s+does\b", r"^how\s+do\b", r"^how\s+to\b", r"\bbest\s+practices?\b", r"^tutorial\b", r"^guide\s+to\b", r"^learn\s+about\b", r"^introduction\s+to\b", r"^basics\s+of\b", ] + _COMPILED_PATTERNS = [re.compile(p, re.IGNORECASE) for p in EDUCATIONAL_PATTERNS] def __init__(self): """Initialize the learning tracker.""" - self._compiled_patterns = [re.compile(p, re.IGNORECASE) for p in self.EDUCATIONAL_PATTERNS] + pass # No instance-specific initialization needed def is_educational_query(self, question: str) -> bool: """Determine if a question is educational in nature.""" - for pattern in self._compiled_patterns: + for pattern in self._COMPILED_PATTERNS: if pattern.search(question): return True return False
172-200: Consider clarifying topic truncation logic.The truncation logic at lines 198-199 works correctly but could be more explicit about its intent to truncate at word boundaries.
🔎 Proposed clarification
# Clean up and truncate topic = topic.strip("? ").strip() - # Take first 50 chars as topic identifier if len(topic) > 50: + # Truncate at word boundary to avoid splitting words topic = topic[:50].rsplit(" ", 1)[0] return topic
202-225: Consider using timezone-aware timestamps.Lines 216 and 221 use
datetime.now().isoformat()which creates naive (timezone-unaware) timestamps. For better consistency and sorting accuracy, consider using UTC timestamps.🔎 Proposed improvement
+from datetime import datetime, timezone def record_topic(self, question: str) -> None: """Record that the user explored an educational topic.""" if not self.is_educational_query(question): return topic = self.extract_topic(question) if not topic: return history = self._load_history() # Update or add topic if topic in history["topics"]: history["topics"][topic]["count"] += 1 - history["topics"][topic]["last_accessed"] = datetime.now().isoformat() + history["topics"][topic]["last_accessed"] = datetime.now(timezone.utc).isoformat() else: history["topics"][topic] = { "count": 1, - "first_accessed": datetime.now().isoformat(), - "last_accessed": datetime.now().isoformat(), + "first_accessed": datetime.now(timezone.utc).isoformat(), + "last_accessed": datetime.now(timezone.utc).isoformat(), } history["total_queries"] = history.get("total_queries", 0) + 1 self._save_history(history)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cortex/ask.pycortex/cli.pydocs/COMMANDS.mdtests/test_ask.py
🚧 Files skipped from review as they are similar to previous changes (2)
- cortex/cli.py
- docs/COMMANDS.md
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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:
tests/test_ask.pycortex/ask.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_ask.py
🧬 Code graph analysis (1)
tests/test_ask.py (2)
cortex/ask.py (9)
ask(455-520)SystemInfoGatherer(20-136)record_topic(202-225)get_history(227-229)get_recent_topics(231-242)get_recent_topics(530-539)get_learning_history(522-528)gather_context(129-136)_get_system_prompt(331-388)cortex/llm/interpreter.py (1)
_get_system_prompt(123-157)
⏰ 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). (1)
- GitHub Check: Agent
🔇 Additional comments (9)
cortex/ask.py (6)
11-11: LGTM! Imports are appropriate.The new imports (
re,datetime,Path) are all used by theLearningTrackerclass and follow proper import conventions.Also applies to: 15-16
285-285: LGTM! Clean integration of learning tracker.The
LearningTrackeris properly instantiated in__init__, maintaining good separation of concerns.
331-388: LGTM! Comprehensive system prompt enhancements.The enhanced system prompt effectively distinguishes between educational and diagnostic queries with clear formatting rules. The terminal-friendly formatting guidelines (avoiding markdown headings, using bold for sections) are well thought out.
398-398: LGTM! Appropriate token limit increase.Increasing
max_tokensfrom 500 to 2000 is justified for educational responses that require structured tutorials and code examples.Also applies to: 410-410
517-519: LGTM! Proper placement of topic recording.Recording the topic after the response is generated and cached is the correct approach, as it only tracks successful queries.
522-539: LGTM! Well-documented public API.Both methods follow coding guidelines with proper type hints and docstrings. The delegation pattern to
learning_trackermaintains good encapsulation.tests/test_ask.py (3)
265-388: LGTM! Comprehensive test coverage for LearningTracker.The test class thoroughly covers:
- Educational query pattern detection (multiple trigger phrases)
- Topic extraction with various prefixes
- Edge cases like truncation and empty states
- Persistence and data integrity
- Proper isolation using temp files and patching
The tests are well-structured with clear naming and proper setup/teardown.
390-451: LGTM! Solid integration tests for learning features.The tests properly verify the integration between
AskHandlerandLearningTracker:
- Educational topics are recorded through the
ask()method- Diagnostic queries are correctly ignored
- Public API methods work as expected
- System prompt content is validated
Using the fake provider and disabling cache ensures fast, deterministic tests.
454-485: LGTM! System prompt validation tests.These tests verify that the enhanced system prompt contains the necessary instructions for both educational and diagnostic queries. The tests appropriately check for key phrases that indicate the prompt's intent.
Note: These tests validate prompt content directly, which means they may need updates if prompt wording changes, but this is acceptable for ensuring the educational feature works as designed.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @cortex/ask.py:
- Around line 526-529: The issue is that
self.learning_tracker.record_topic(question) is only called after generating
non-cached answers, so cached hits (returned around the earlier cache return)
never update last_accessed/count; move the call to
self.learning_tracker.record_topic(question) so it runs for every access—either
invoke it immediately before the cache lookup/early-return (so both cache hits
and misses are recorded) or call it right before any early return that returns a
cached response (ensure it executes in the same function/method where the cache
is checked, preserving the original variable names like cached_answer/cache_hit
and the existing return behavior).
🧹 Nitpick comments (3)
cortex/ask.py (2)
163-165: Missing docstring content.Per coding guidelines, docstrings are required for all public APIs. The
__init__method has a docstring but it's minimal. Consider adding a brief note about pattern compilation.
257-264: Consider logging on save failure for debuggability.Silent failure when unable to write learning history (
passon line 264) is reasonable for CLI UX, but makes diagnosing permission or disk issues difficult. Consider adding debug-level logging if a logger is available.tests/test_ask.py (1)
417-468: Consider cleaning upCORTEX_FAKE_RESPONSEin tearDown.Tests set
os.environ["CORTEX_FAKE_RESPONSE"]but don't clean it up. This could cause test pollution if test order changes or new tests are added. Consider adding cleanup intearDownor usingunittest.mock.patch.dict.Proposed fix
def setUp(self): """Set up test fixtures.""" self.temp_dir = tempfile.mkdtemp() self.temp_file = Path(self.temp_dir) / "learning_history.json" self.patcher = patch.object(LearningTracker, "PROGRESS_FILE", self.temp_file) self.patcher.start() + self._original_fake_response = os.environ.get("CORTEX_FAKE_RESPONSE") def tearDown(self): """Clean up temporary files.""" import shutil self.patcher.stop() + if self._original_fake_response is not None: + os.environ["CORTEX_FAKE_RESPONSE"] = self._original_fake_response + else: + os.environ.pop("CORTEX_FAKE_RESPONSE", None) if os.path.exists(self.temp_dir): shutil.rmtree(self.temp_dir)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cortex/ask.pycortex/cli.pydocs/COMMANDS.mdtests/test_ask.py
🚧 Files skipped from review as they are similar to previous changes (2)
- cortex/cli.py
- docs/COMMANDS.md
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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:
tests/test_ask.pycortex/ask.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_ask.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). (4)
- GitHub Check: Test (Python 3.11)
- GitHub Check: test (3.10)
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
🔇 Additional comments (7)
cortex/ask.py (4)
1-17: LGTM!Module docstring and imports are appropriately updated to reflect the new educational capabilities.
287-287: LGTM!Clean integration of
LearningTrackerintoAskHandler.
406-408: Verify cost implications of 4x token increase.Increasing
max_tokensfrom 500 to 2000 is appropriate for educational content but increases potential API costs. Ensure this aligns with expected usage patterns and cost budgets.Also applies to: 418-420
531-548: LGTM!Well-documented public APIs with proper type hints and docstrings. Clean delegation to
LearningTracker.tests/test_ask.py (3)
292-311: Good test isolation with temporary directory and patched PROGRESS_FILE.The setup properly isolates tests from the real learning history file, and teardown ensures cleanup.
481-512: LGTM!Good test coverage for the enhanced system prompt content, verifying presence of key sections and instructions.
292-512: Comprehensive test coverage for new learning features.The 25 new tests thoroughly cover:
- Educational query detection patterns
- Topic extraction logic
- History persistence and retrieval
- Integration with AskHandler
- System prompt content validation
Good adherence to the >80% coverage guideline for new functionality.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
cortex/ask.py (4)
163-165: Missing return type hint on__init__.Per coding guidelines, type hints are required. Add
-> Nonereturn type.- def __init__(self): + def __init__(self) -> None: """Initialize the learning tracker.""" self._compiled_patterns = [re.compile(p, re.IGNORECASE) for p in self.EDUCATIONAL_PATTERNS]
251-253: Specify explicit encoding for file operations.Reading JSON without explicit encoding may cause issues on systems where the default encoding is not UTF-8. The same applies to the write operation on line 261.
Suggested fix
- with open(self.PROGRESS_FILE) as f: + with open(self.PROGRESS_FILE, encoding="utf-8") as f: return json.load(f)And for
_save_history:- with open(self.PROGRESS_FILE, "w") as f: + with open(self.PROGRESS_FILE, "w", encoding="utf-8") as f: json.dump(history, f, indent=2)
144-144: Consider lazy initialization ofPROGRESS_FILEfor robustness in containerized environments.
Path.home()is evaluated at class definition time, which could raiseRuntimeErrorin environments whereHOMEis not set (e.g., some containers). While this pattern is used consistently throughout the codebase, consider making this a property or initializing in__init__for improved compatibility.
431-451: Addnum_predict: 2000to Ollama options for token limit consistency.OpenAI and Claude both use
max_tokens=2000, but_call_ollamadoesn't specify a token limit (Ollama defaults to unlimited generation). For consistency across all three providers, addnum_predict: 2000to the Ollama options.Suggested change
data = json.dumps( { "model": self.model, "prompt": prompt, "stream": False, - "options": {"temperature": 0.3}, + "options": {"temperature": 0.3, "num_predict": 2000}, } ).encode("utf-8")
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cortex/ask.py
🧰 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/ask.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). (4)
- GitHub Check: Build Package
- GitHub Check: test (3.12)
- GitHub Check: test (3.11)
- GitHub Check: test (3.10)
🔇 Additional comments (2)
cortex/ask.py (2)
496-498: LGTM!Good to track topic access even for cached responses - this correctly reflects user engagement with educational topics over time, not just unique queries.
533-550: LGTM!Clean delegation methods with proper type hints and docstrings as required by the coding guidelines.
Record topic access even when returning cached responses to ensure last_accessed timestamps and counts are updated correctly.
ShreeJejurikar
force-pushed
the
issue-393
branch
from
c553df7 to
aea5339
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @cortex/ask.py:
- Around line 141-165: PROGRESS_FILE is computed at import time which can raise
RuntimeError in restricted envs; change it to lazy initialization by removing
the module-level Path.home() usage and implement a property or cached_property
on LearningTracker (e.g., progress_file) that computes Path.home() / ".cortex" /
"learning_history.json" inside a try/except, falling back to a safe location
(like tempfile.gettempdir() or Path.cwd()) and ensuring parent dirs are created;
update methods that reference PROGRESS_FILE to use the new progress_file
property and add minimal error handling/logging when home is inaccessible.
🧹 Nitpick comments (2)
cortex/ask.py (2)
167-172: Consider usingany()for more idiomatic Python.The loop can be simplified using
any()for improved readability.Suggested refactor
def is_educational_query(self, question: str) -> bool: """Determine if a question is educational in nature.""" - for pattern in self._compiled_patterns: - if pattern.search(question): - return True - return False + return any(pattern.search(question) for pattern in self._compiled_patterns)
257-264: Silent failure on save may hide persistent issues.While silent failure keeps CLI output clean, users won't know if their learning history is never being saved (e.g., due to permissions). Consider logging at debug level or surfacing persistent failures after repeated attempts.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cortex/ask.py
🧰 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/ask.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). (4)
- GitHub Check: Test (Python 3.10)
- GitHub Check: test (3.10)
- GitHub Check: test (3.12)
- GitHub Check: test (3.11)
🔇 Additional comments (7)
cortex/ask.py (7)
1-17: LGTM!The module docstring accurately reflects the expanded scope, and the new imports (
re,datetime,Path) are appropriately added for theLearningTrackerfunctionality.
204-227: LGTM!The method has proper guard clauses for non-educational queries and empty topics. The history update logic correctly handles both new and existing topics.
287-287: LGTM!The
LearningTrackerintegration intoAskHandler.__init__is clean and follows the existing initialization pattern.
340-397: Well-structured dual-mode system prompt.The enhanced prompt effectively guides the LLM to distinguish between educational and diagnostic queries, with clear formatting rules for terminal-friendly output. The example formatting section is a good practice for LLM instruction.
407-407: max_tokens increase supports longer educational responses.The 4x increase from 500 to 2000 tokens aligns with the PR objective to enable tutorial-style content. Be aware this may increase API costs and response latency for commercial providers.
Also applies to: 419-419
496-498: LGTM!Topic tracking is correctly integrated in both the cache-hit and fresh-response paths. The
record_topicmethod internally filters non-educational queries, so calling it unconditionally is appropriate.Also applies to: 528-529
533-550: LGTM!The new public API methods are well-documented with proper docstrings and type hints. They provide clean access to learning history functionality at the
AskHandlerlevel.
cortex/ask.py
Outdated
| class LearningTracker: | ||
| """Tracks educational topics the user has explored.""" | ||
|
|
||
| PROGRESS_FILE = Path.home() / ".cortex" / "learning_history.json" | ||
|
|
||
| # Patterns that indicate educational questions | ||
| EDUCATIONAL_PATTERNS = [ | ||
| r"^explain\b", | ||
| r"^teach\s+me\b", | ||
| r"^what\s+is\b", | ||
| r"^what\s+are\b", | ||
| r"^how\s+does\b", | ||
| r"^how\s+do\b", | ||
| r"^how\s+to\b", | ||
| r"\bbest\s+practices?\b", | ||
| r"^tutorial\b", | ||
| r"^guide\s+to\b", | ||
| r"^learn\s+about\b", | ||
| r"^introduction\s+to\b", | ||
| r"^basics\s+of\b", | ||
| ] | ||
|
|
||
| def __init__(self): | ||
| """Initialize the learning tracker.""" | ||
| self._compiled_patterns = [re.compile(p, re.IGNORECASE) for p in self.EDUCATIONAL_PATTERNS] |
There was a problem hiding this comment.
Path.home() evaluated at class definition may fail in restricted environments.
PROGRESS_FILE is evaluated at import time. If HOME is unset or inaccessible (e.g., containerized/sandboxed environments), Path.home() raises RuntimeError, crashing the import. Consider lazy initialization or wrapping in a property/method with error handling.
Suggested fix using a cached property
class LearningTracker:
"""Tracks educational topics the user has explored."""
- PROGRESS_FILE = Path.home() / ".cortex" / "learning_history.json"
+ _progress_file: Path | None = None
+ @classmethod
+ def _get_progress_file(cls) -> Path | None:
+ """Get the progress file path, handling missing HOME gracefully."""
+ if cls._progress_file is None:
+ try:
+ cls._progress_file = Path.home() / ".cortex" / "learning_history.json"
+ except RuntimeError:
+ return None
+ return cls._progress_file🤖 Prompt for AI Agents
In @cortex/ask.py around lines 141 - 165, PROGRESS_FILE is computed at import
time which can raise RuntimeError in restricted envs; change it to lazy
initialization by removing the module-level Path.home() usage and implement a
property or cached_property on LearningTracker (e.g., progress_file) that
computes Path.home() / ".cortex" / "learning_history.json" inside a try/except,
falling back to a safe location (like tempfile.gettempdir() or Path.cwd()) and
ensuring parent dirs are created; update methods that reference PROGRESS_FILE to
use the new progress_file property and add minimal error handling/logging when
home is inaccessible.
✅ Addressed in commit 11e7b35
ShreeJejurikar
force-pushed
the
issue-393
branch
from
aea5339 to
f4abbed
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @docs/COMMANDS.md:
- Around line 76-133: Docs mention a --offline flag for the cortex ask command
but the CLI parser (cortex/cli.py) and AskHandler.ask() don't support it; either
remove the doc line or implement the flag. To implement: add an optional boolean
--offline/--no-offline flag to the ask command definition in cortex/cli.py that
sets a parameter (e.g., offline: bool) and pass that value when invoking
AskHandler.ask; update AskHandler.ask(signature) to accept an offline: bool
parameter and branch its behavior to only use cached responses when offline is
true (or raise an informative error if caching isn't available). Ensure help
text and COMMANDS.md are consistent with the new flag.
🧹 Nitpick comments (1)
cortex/ask.py (1)
204-227: Consider thread safety for concurrent file access.The
record_topicmethod performs a read-modify-write cycle on the history file without locking. If multiplecortex askprocesses run concurrently, this could lead to lost updates. Given this is a CLI tool where concurrent invocations are rare and learning history is non-critical, this is acceptable for now.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cortex/ask.pycortex/cli.pydocs/COMMANDS.mdtests/test_ask.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/cli.pycortex/ask.pytests/test_ask.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_ask.py
🧬 Code graph analysis (1)
tests/test_ask.py (1)
cortex/ask.py (13)
ask(464-531)AskHandler(267-550)LearningTracker(141-264)SystemInfoGatherer(22-138)is_educational_query(167-172)extract_topic(174-202)record_topic(204-227)get_history(229-231)get_recent_topics(233-244)get_recent_topics(541-550)get_learning_history(533-539)gather_context(131-138)_get_system_prompt(340-397)
⏰ 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 Package
- GitHub Check: test (3.11)
- GitHub Check: test (3.10)
- GitHub Check: test (3.12)
🔇 Additional comments (24)
cortex/cli.py (2)
9-10: LGTM!Import correctly added to enable Markdown rendering for ask command responses.
617-618: LGTM!The Markdown rendering correctly formats the LLM response for terminal display, aligning with the enhanced system prompt that instructs the LLM to use terminal-friendly formatting (bold text, code blocks, etc.).
cortex/ask.py (12)
4-6: LGTM!Module docstring properly updated to reflect the new educational content and learning progress tracking capabilities.
11-16: LGTM!Necessary imports added for the new LearningTracker functionality:
refor pattern matching,datetimefor timestamps, andPathfor file operations.
141-165: LGTM!The
LearningTrackerclass is well-structured with:
- Clear docstring explaining its purpose
- Sensible file location (
~/.cortex/learning_history.json)- Comprehensive regex patterns for educational query detection
- Pre-compiled patterns for better performance
167-172: LGTM!Educational query detection correctly iterates through compiled patterns and returns on first match for efficiency.
174-202: LGTM!Topic extraction logic is sound:
- Removes common educational prefixes to isolate the actual topic
- Properly handles truncation for long topics (50 char limit)
- Uses word-boundary-aware truncation (
rsplit(" ", 1)[0]) to avoid cutting mid-word
246-264: LGTM!File I/O methods have appropriate error handling:
_load_historyreturns sensible defaults on missing file or parse errors_save_historycreates parent directories and silently fails on write errors to avoid disrupting the user experience
287-287: LGTM!
LearningTrackeris properly initialized as a public attribute inAskHandler.__init__, enabling the learning history features.
340-397: Well-designed system prompt enhancement.The prompt effectively:
- Distinguishes between educational and diagnostic queries with clear trigger examples
- Provides structured guidance for each response type
- Includes critical terminal formatting rules (avoiding
#headings, using bold instead)- Shows a concrete formatting example
This aligns well with the PR objective of automatic intent detection.
407-407: Verify token increase impact on API costs.The
max_tokensincrease from 500 to 2000 (4x) for OpenAI will proportionally increase costs for educational responses. This is intentional per the PR objectives, but ensure users are aware of potential cost implications.
419-419: LGTM!Matching
max_tokensincrease for Claude provider to maintain consistent behavior across providers.
496-498: LGTM!Correctly tracks topic access even for cached responses, ensuring learning history remains accurate regardless of cache hits.
528-550: LGTM!
- Topic tracking for non-cached responses is correctly placed after the LLM call
- New public API methods
get_learning_history()andget_recent_topics()are properly documented with docstrings and type hints, as per coding guidelinesdocs/COMMANDS.md (2)
11-11: LGTM!Quick reference updated with clear, concise description of the enhanced
cortex askfunctionality.
525-541: LGTM!The "Learning with Cortex Ask" workflow section provides a clear, practical guide for users, demonstrating the progression from diagnostic queries to educational content, with a helpful note about automatic learning history tracking.
tests/test_ask.py (8)
3-3: LGTM!Import added for JSON operations used in learning history tests.
13-13: LGTM!
LearningTrackercorrectly imported alongside existingAskHandlerandSystemInfoGathererimports.
292-311: LGTM!Test fixture setup properly:
- Uses
tempfile.mkdtemp()for isolated test directory- Patches
LearningTracker.PROGRESS_FILEto use temp location- Cleans up in
tearDownwith proper error handling for Windows
312-346: LGTM!Comprehensive test coverage for
is_educational_query:
- Tests all major educational patterns (explain, teach me, what is, how does, best practices, tutorial)
- Includes negative test cases for diagnostic queries
- Good variety of case sensitivity tests
347-367: LGTM!Good topic extraction tests covering prefix removal and truncation behavior.
368-415: LGTM!Solid tests for
record_topicand related functionality:
- File creation verification
- Data storage correctness
- Count increment on repeated topics
- Non-educational query filtering
- Recent topics retrieval with ordering
- Total queries tracking
417-478: LGTM!Integration tests properly verify
AskHandler's learning features:
- Educational topics are recorded after ask
- Diagnostic questions don't pollute learning history
get_recent_topicsAPI works through handler- System prompt contains expected educational/diagnostic instructions
481-512: LGTM!System prompt enhancement tests verify the prompt contains:
- Query type detection section
- Educational trigger phrases
- Code examples and best practices guidance
- System context and actionable response instructions
| ### `cortex ask` | ||
|
|
||
| Ask natural language questions about your system or learn about Linux, packages, and best practices. The AI automatically detects whether you're asking a diagnostic question about your system or an educational question to learn something new. | ||
|
|
||
| **Usage:** | ||
| ```bash | ||
| cortex ask "<question>" | ||
| ``` | ||
|
|
||
| **Question Types:** | ||
|
|
||
| **Diagnostic Questions** - Questions about your specific system: | ||
| ```bash | ||
| # System status queries | ||
| cortex ask "why is my disk full" | ||
| cortex ask "what packages need updating" | ||
| cortex ask "is my Python version compatible with TensorFlow" | ||
| cortex ask "check my GPU drivers" | ||
| ``` | ||
|
|
||
| **Educational Questions** - Learn about Linux, packages, and best practices: | ||
| ```bash | ||
| # Explanations and tutorials | ||
| cortex ask "explain how Docker containers work" | ||
| cortex ask "what is systemd and how do I use it" | ||
| cortex ask "teach me about nginx configuration" | ||
| cortex ask "best practices for securing a Linux server" | ||
| cortex ask "how to set up a Python virtual environment" | ||
| ``` | ||
|
|
||
| **Features:** | ||
| - **Automatic Intent Detection**: The AI distinguishes between diagnostic and educational queries | ||
| - **System-Aware Responses**: Uses your actual system context (OS, Python version, GPU, etc.) | ||
| - **Structured Learning**: Educational responses include examples, best practices, and related topics | ||
| - **Learning Progress Tracking**: Educational topics you explore are tracked in `~/.cortex/learning_history.json` | ||
| - **Response Caching**: Repeated questions return cached responses for faster performance | ||
|
|
||
| **Examples:** | ||
| ```bash | ||
| # Diagnostic: Get specific info about your system | ||
| cortex ask "what version of Python do I have" | ||
| cortex ask "can I run PyTorch on this system" | ||
|
|
||
| # Educational: Learn with structured tutorials | ||
| cortex ask "explain how apt package management works" | ||
| cortex ask "what are best practices for Docker security" | ||
| cortex ask "guide to setting up nginx as a reverse proxy" | ||
|
|
||
| # Mix of both | ||
| cortex ask "how do I install and configure Redis" | ||
| ``` | ||
|
|
||
| **Notes:** | ||
| - Educational responses are longer and include code examples with syntax highlighting | ||
| - The `--offline` flag can be used to only return cached responses | ||
| - Learning history helps track what topics you've explored over time | ||
|
|
||
| --- |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for --offline flag references in Python files
echo "=== Searching for --offline in cortex/cli.py ==="
rg -n '\-\-offline' cortex/cli.py
echo -e "\n=== Searching for offline in cortex/ask.py ==="
rg -n 'offline' cortex/ask.py
echo -e "\n=== Checking cortex/cli.py around lines 1748-1749 ==="
if [ -f cortex/cli.py ]; then
sed -n '1740,1760p' cortex/cli.py
fi
echo -e "\n=== Checking if cortex/ask.py exists ==="
if [ -f cortex/ask.py ]; then
wc -l cortex/ask.py
else
echo "cortex/ask.py not found"
# Try to find similar files
find . -name "*ask*" -type f | head -20
fiRepository: cortexlinux/cortex
Length of output: 1270
🏁 Script executed:
#!/bin/bash
# Find the ask command parser definition
echo "=== Searching for ask command subparser definition ==="
rg -n "ask" cortex/cli.py | grep -i "subparser\|add_parser\|argument" | head -20
echo -e "\n=== Searching for ask in cortex/cli.py ==="
rg -n "add_parser.*ask|subparsers.*ask" cortex/cli.py
echo -e "\n=== Checking cortex/ask.py around line 475 ==="
sed -n '470,490p' cortex/ask.py
echo -e "\n=== Checking entire ask command definition in cli.py ==="
rg -B5 -A10 "ask.*command|def.*ask" cortex/cli.py | head -50
echo -e "\n=== Checking COMMANDS.md line 130 ==="
if [ -f docs/COMMANDS.md ]; then
sed -n '125,135p' docs/COMMANDS.md
fiRepository: cortexlinux/cortex
Length of output: 2036
🏁 Script executed:
#!/bin/bash
# Get full ask_parser definition
echo "=== Full ask_parser definition in cli.py ==="
sed -n '2137,2160p' cortex/cli.py
echo -e "\n=== Check ask function signature ==="
rg -A5 "def ask\(self" cortex/cli.py | head -20
echo -e "\n=== Search for offline usage in cortex/ask.py ==="
rg -B3 -A3 "offline" cortex/ask.py
echo -e "\n=== Check how ask command calls handler ==="
rg -A15 "def ask\(self, question" cortex/cli.py | head -40Repository: cortexlinux/cortex
Length of output: 2269
🏁 Script executed:
#!/bin/bash
# Check AskHandler.ask() method signature
echo "=== AskHandler.ask() method definition ==="
rg -B2 -A20 "class AskHandler" cortex/ask.py | head -30
echo -e "\n=== ask() method in AskHandler ==="
rg -B2 -A15 "def ask\(self, question" cortex/ask.pyRepository: cortexlinux/cortex
Length of output: 1345
Correct documentation or implement the --offline flag for the ask command.
Line ~131 of the documentation states: "The --offline flag can be used to only return cached responses"
However, the ask command parser in cortex/cli.py (lines 2137-2138) only defines question as a positional argument with no --offline option. The AskHandler.ask() method also does not accept an offline parameter. Either implement the flag or remove this documentation.
🤖 Prompt for AI Agents
In @docs/COMMANDS.md around lines 76 - 133, Docs mention a --offline flag for
the cortex ask command but the CLI parser (cortex/cli.py) and AskHandler.ask()
don't support it; either remove the doc line or implement the flag. To
implement: add an optional boolean --offline/--no-offline flag to the ask
command definition in cortex/cli.py that sets a parameter (e.g., offline: bool)
and pass that value when invoking AskHandler.ask; update
AskHandler.ask(signature) to accept an offline: bool parameter and branch its
behavior to only use cached responses when offline is true (or raise an
informative error if caching isn't available). Ensure help text and COMMANDS.md
are consistent with the new flag.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @cortex/ask.py:
- Around line 1-6: The file cortex/ask.py fails Black formatting in CI; run the
formatter (black .) locally or apply Black's rules to reformat cortex/ask.py
(and any other changed files), ensure the module docstring and surrounding code
conform to Black's line-length and string conventions, then stage and commit the
reformatted file(s) so CI picks up the updated, Black-compliant version.
🧹 Nitpick comments (2)
cortex/ask.py (1)
163-166: Add type hint for_compiled_patterns.Per coding guidelines, type hints are required. Consider adding the type annotation:
def __init__(self): """Initialize the learning tracker.""" - self._compiled_patterns = [re.compile(p, re.IGNORECASE) for p in self.EDUCATIONAL_PATTERNS] + self._compiled_patterns: list[re.Pattern[str]] = [ + re.compile(p, re.IGNORECASE) for p in self.EDUCATIONAL_PATTERNS + ]tests/test_ask.py (1)
292-413: Consider adding edge case tests for error handling.The
LearningTrackerhandles corrupted JSON files and inaccessible home directories, but these paths aren't tested. Consider adding tests for:
_load_historywith malformed JSON (should return empty dict)progress_filefallback whenPath.home()raisesRuntimeError
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cortex/ask.pytests/test_ask.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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:
tests/test_ask.pycortex/ask.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_ask.py
🪛 GitHub Actions: CI
cortex/ask.py
[error] 1-1: Black formatting check failed. 1 file would be reformatted. Run 'black .' to fix formatting.
[error] 1-1: Command failed with exit code 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). (3)
- GitHub Check: test (3.12)
- GitHub Check: test (3.11)
- GitHub Check: test (3.10)
🔇 Additional comments (14)
cortex/ask.py (7)
186-214: LGTM!The topic extraction logic handles prefixes cleanly, and the truncation at 50 characters using
rsplitpreserves word boundaries appropriately.
216-239: LGTM!The topic recording logic correctly handles both new and existing topics with proper timestamp tracking.
269-276: LGTM!Silent failure on save is acceptable for non-critical learning history. Creating parent directories defensively is good practice.
352-409: Well-designed dual-mode system prompt.The prompt structure clearly delineates educational vs. diagnostic handling with concrete examples and terminal-friendly formatting rules. The example formatting section helps guide consistent LLM output.
419-431: Verify cost implications of increasedmax_tokens.The 4x increase (500 → 2000) enables longer educational responses as intended. Ensure this aligns with expected API usage costs, especially for high-volume deployments.
508-541: LGTM!Topic recording is correctly placed to execute exactly once per
ask()call, regardless of whether the response is cached or freshly generated.
545-562: LGTM!Clean delegation to
learning_trackerwith proper type hints and complete docstrings.tests/test_ask.py (7)
1-14: LGTM!Imports are appropriate for the new test cases.
292-308: LGTM!Test setup properly isolates the learning history file to a temp directory, and teardown ensures cleanup.
310-344: LGTM!Good coverage of educational pattern detection with both positive and negative test cases.
345-365: LGTM!Topic extraction tests cover key prefixes and verify truncation behavior.
366-413: LGTM!Comprehensive tests for topic recording, including file creation, count increments, and recent topics ordering.
415-478: LGTM!Integration tests properly verify the learning tracker behavior through
AskHandler, covering both educational and diagnostic query paths.
480-511: LGTM!System prompt tests appropriately verify the presence of key sections without being overly brittle to prompt wording changes.
| """Natural language query interface for Cortex. | ||
| Handles user questions about installed packages, configurations, | ||
| and system state using LLM with semantic caching. | ||
| and system state using LLM with semantic caching. Also provides | ||
| educational content and tracks learning progress. | ||
| """ |
There was a problem hiding this comment.
Fix Black formatting to pass CI.
The pipeline indicates Black formatting failed. Run black . locally and commit the reformatted code to fix the CI failure.
🧰 Tools
🪛 GitHub Actions: CI
[error] 1-1: Black formatting check failed. 1 file would be reformatted. Run 'black .' to fix formatting.
[error] 1-1: Command failed with exit code 1.
🤖 Prompt for AI Agents
In @cortex/ask.py around lines 1 - 6, The file cortex/ask.py fails Black
formatting in CI; run the formatter (black .) locally or apply Black's rules to
reformat cortex/ask.py (and any other changed files), ensure the module
docstring and surrounding code conform to Black's line-length and string
conventions, then stage and commit the reformatted file(s) so CI picks up the
updated, Black-compliant version.
Convert PROGRESS_FILE from class-level constant to lazily-computed property to avoid RuntimeError in restricted environments where Path.home() may fail at import time. Falls back to temp directory.
ShreeJejurikar
force-pushed
the
issue-393
branch
from
dd254a3 to
11e7b35
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cortex/ask.py (1)
219-279: Consider adding a brief note about concurrent access.The read-modify-write pattern in
record_topicisn't atomic, so concurrent processes could lose updates. This is acceptable for a single-user CLI tool, but consider adding a brief code comment noting this limitation if the feature might be used in multi-process scenarios in the future.tests/test_ask.py (1)
3-3: Unused import.The
jsonmodule is imported but not used in this test file. TheLearningTrackerclass handles JSON operations internally.♻️ Proposed fix
"""Unit tests for the ask module.""" -import json import os import sys
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cortex/ask.pytests/test_ask.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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:
tests/test_ask.pycortex/ask.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_ask.py
🧬 Code graph analysis (1)
tests/test_ask.py (1)
cortex/ask.py (11)
ask(479-546)LearningTracker(141-279)SystemInfoGatherer(22-138)is_educational_query(182-187)extract_topic(189-217)record_topic(219-242)get_history(244-246)get_recent_topics(248-259)get_recent_topics(556-565)gather_context(131-138)_get_system_prompt(355-412)
⏰ 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: test (3.12)
- GitHub Check: test (3.10)
- GitHub Check: test (3.11)
🔇 Additional comments (10)
cortex/ask.py (7)
1-17: LGTM!Imports are properly organized and the module docstring has been updated to reflect the new educational content and learning progress tracking capabilities.
141-180: LGTM!The
LearningTrackerclass is well-designed with:
- Lazy initialization of the progress file path with a sensible fallback for restricted environments
- Compiled regex patterns for efficient repeated matching
- Proper type hints and docstrings on all public methods
182-217: LGTM!The educational query detection and topic extraction logic is clean and handles edge cases well, including the word-boundary-aware truncation at 50 characters.
282-312: LGTM!The
LearningTrackerintegration intoAskHandleris clean. The tracker is properly initialized in__init__and exposes appropriate public methods.
355-412: LGTM!The enhanced system prompt is well-structured with clear sections for query type detection, educational vs. diagnostic response guidelines, and terminal-friendly formatting rules. The formatting guidance (avoiding markdown headings, using bold text instead) aligns with the PR objective of improving terminal rendering.
414-444: LGTM!The
max_tokensincrease from 500 to 2000 is applied consistently to both OpenAI and Claude providers, enabling longer educational responses as specified in the PR objectives.
479-565: LGTM!The
askmethod properly records topics for both cached and fresh responses, ensuring learning history is tracked regardless of cache state. The newget_learning_historyandget_recent_topicsmethods provide clean public APIs with proper docstrings and type hints.tests/test_ask.py (3)
292-413: LGTM!The
TestLearningTrackerclass provides comprehensive test coverage including:
- Educational query detection for all pattern types
- Topic extraction with prefix removal and truncation
- History persistence (file creation, data storage, count incrementing)
- Edge cases (non-educational queries, empty history, recent topics ordering)
Using a temporary directory with direct
_progress_fileassignment is an appropriate strategy for test isolation.
415-477: LGTM!The
TestAskHandlerLearningclass effectively tests the integration betweenAskHandlerandLearningTracker, verifying that:
- Educational questions are recorded after
ask()calls- Diagnostic questions are not recorded
- The public API methods (
get_learning_history,get_recent_topics) work through the handler- The system prompt contains expected educational guidance
480-511: LGTM!The
TestSystemPromptEnhancementclass provides good regression protection for the enhanced system prompt, verifying the presence of key sections (query type detection, educational instructions, diagnostic instructions) and content (code examples, best practices, related topics).
Anshgrover23
left a comment
Anshgrover23
left a comment
There was a problem hiding this comment.
@ShreeJejurikar Kindly address all bot comments and ping me again will review then.
This commit resolves 18 outstanding issues identified in code review: CODE QUALITY IMPROVEMENTS: - Removed unused 'import json' from test_ask.py - Added return type hints (-> None) to LearningTracker.__init__ - Added type annotation for _compiled_patterns: list[re.Pattern[str]] - Enhanced __init__ docstring with pattern compilation details DESIGN & LOGIC ENHANCEMENTS: - Refactored _compiled_patterns as class variable to avoid regex recompilation - Improved topic truncation logic with explicit boundary checking (len > 50) - Switched to UTC timestamps (datetime.now(timezone.utc).isoformat()) - Simplified is_educational_query() using any() for idiomatic Python ERROR HANDLING & ROBUSTNESS: - Added explicit UTF-8 encoding to json.load() and json.dump() - Added debug-level logging for save failures without breaking CLI - Fixed test environment pollution by cleaning up CORTEX_FAKE_RESPONSE - Verified lazy Path.home() initialization with RuntimeError fallback CONSISTENCY & DOCUMENTATION: - Removed obsolete --offline flag documentation reference - Replaced markdown headings with bold text in system prompt for consistency - Added num_predict: 2000 to Ollama options matching OpenAI/Claude tokens - Added edge case tests for malformed JSON and Path.home() failures - Added concurrency documentation notes in record_topic() method - Formatted all changes with Black and Ruff TESTING: - All 51 tests passing (+ 2 new edge case tests) - Black formatter: ✓ All 129 files properly formatted - Ruff linter: ✓ All checks passed Closes: Related to enhanced cortex ask with tutor capabilities
- Renamed 'gpu' variable to 'gpu_info' for better clarity - Now consistently uses 'gpu_info' throughout instead of 'gpu' - Improves code readability and consistency
This reverts commit 3990c62.
|
4 participants
Related Issue
Closes #393
Summary
Enhances the existing
cortex askcommand to include tutor-like capabilities without adding a separate command or flag. The LLM automatically detects whether a question is educational or diagnostic and responds appropriately.Changes:
LearningTrackerclass to track educational topics explored by the user~/.cortex/learning_history.jsonmax_tokensfrom 500 to 2000 for longer educational responsesCOMMANDS.mdwithcortex askdocumentationExample usage:
AI Disclosure
Used Claude Code for development assistance.
Checklist
type(scope): descriptionor[scope] descriptionpytest tests/)Video Demonstration:
Screencast.from.12-01-26.10.42.51.AM.IST.webm
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.