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
|
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 a LearningTracker to detect, extract, and persist educational query topics to Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant AskHandler as AskHandler
participant LearningTracker as LearningTracker
participant FileSystem as FileSystem
participant LLM as LLM
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 system prompt (Educational/Diagnostic) + max_tokens=MAX_TOKENS
LLM-->>AskHandler: answer
AskHandler-->>User: return Markdown-formatted answer
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. ✨ Finishing touches
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.
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.
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
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.
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
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.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@cortex/ask.py`:
- Around line 253-270: The loaded history from _load_history may be malformed or
missing expected keys causing history["topics"] or history["total_queries"] to
KeyError; after calling _load_history() normalize defaults by ensuring
history["topics"] exists as a dict and history["total_queries"] exists as an int
(e.g., set history.setdefault("topics", {}) and
history.setdefault("total_queries", 0)) before updating counts and timestamps,
then proceed to update history["topics"][topic] and increment
history["total_queries"] and finally call _save_history(history).
In `@docs/COMMANDS.md`:
- Around line 108-114: Update the "Response Caching" bullet to indicate caching
is best-effort and may be disabled when `SemanticCache` isn't available; edit
the line that currently reads "Response Caching: Repeated questions return
cached responses for faster performance" to something like "Response Caching
(best-effort): Repeated questions may return cached responses for faster
performance; caching is disabled when `SemanticCache` is not available" so
readers understand it's conditional on the `SemanticCache` dependency.
- Add setdefault() calls after _load_history() to ensure history["topics"] and history["total_queries"] exist, preventing KeyError on malformed data - Update COMMANDS.md to indicate response caching is best-effort and conditional on SemanticCache availability
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cortex/ask.py`:
- Around line 253-258: The loaded learning history from self._load_history() may
be valid JSON but not a dict, causing history.setdefault(...) to raise; update
the code around the history variable to check isinstance(history, dict) and if
not, replace history with a fresh dict (e.g., history = {}) before calling
history.setdefault("topics", {}) and history.setdefault("total_queries", 0) so
malformed non-dict JSON is defensively reset; keep references to _load_history
and the local variable history to locate where to add the type check and reset.
Anshgrover23
left a comment
Anshgrover23
left a comment
There was a problem hiding this comment.
@ShreeJejurikar Kindly address coderabbit comments.
- Reset history to default structure if _load_history() returns non-dict JSON - Add isinstance check to ensure history["topics"] is a dict
|
@Anshgrover23 All bot comments have been resolved, please review again. |
Anshgrover23
left a comment
Anshgrover23
left a comment
There was a problem hiding this comment.
@ShreeJejurikar added some comments.
… constants - Add comprehensive test cases for all 13 EDUCATIONAL_PATTERNS - Fix topic truncation bug when words[0] is empty string - Add proper error handling for malformed/non-dict topic data - Extract hardcoded max_tokens=2000 to MAX_TOKENS constant - Add tests for all new error handling paths - All 77 ask.py tests passing
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cortex/ask.py`:
- Around line 306-317: get_recent_topics currently assumes each entry in topics
is a dict and uses x[1].get("last_accessed", "") which raises AttributeError for
malformed entries; update get_recent_topics to first filter topics.items() to
only include entries where the value is a dict (and optionally contains a
last_accessed key), e.g. use isinstance(value, dict) or a safe accessor before
sorting, so malformed entries are skipped and sorting uses a fallback value;
reference the get_recent_topics method, the topics variable, and
_load_history/record_topic for context.
♻️ Duplicate comments (1)
tests/test_ask.py (1)
399-402: Minor: Consider improving test string grammar.The test string
"what are best practice for nginx"is grammatically awkward. Consider using"what is the best practice for nginx"to still test the singular form while being more natural.Suggested fix
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")) + self.assertTrue(self.tracker.is_educational_query("what is the best practice for nginx"))
- Filter out non-dict topic entries before sorting - Skip entries missing last_accessed key - Add comprehensive test with multiple malformed entry types - Prevents AttributeError when topics dict contains invalid data
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cortex/ask.py`:
- Around line 257-300: The history update can fail if history["total_queries"]
is not an int; update the increment logic after loading/normalizing history (in
the block that uses self._load_history(), history.setdefault(...), and utc_now)
to ensure history["total_queries"] is an integer before adding 1—e.g., check
isinstance(history.get("total_queries"), int) and if not set
history["total_queries"] = 0 (or coerce with int(...) guarded by try/except) and
then do history["total_queries"] += 1; keep using self._save_history(history)
afterwards.
🧹 Nitpick comments (1)
cortex/ask.py (1)
145-239: Consider reusing the existing LearningTracker implementation to avoid drift.There’s already a LearningTracker in
cortex/llm/learning_tracker.py; duplicating it here risks divergence and double maintenance. Consider importing/re-exporting the shared implementation instead.
- Check if total_queries is an int before incrementing - Attempt to coerce string numbers to int - Fallback to 0 if conversion fails or wrong type - Add comprehensive test covering string, list, and convertible values
Anshgrover23
left a comment
Anshgrover23
left a comment
There was a problem hiding this comment.
@ShreeJejurikar Update the video in the pr descripton.
|
@Anshgrover23 The video demonstration in the description has been updated. |
Anshgrover23
left a comment
Anshgrover23
left a comment
There was a problem hiding this comment.
@ShreeJejurikar MAX_TOKENS is semantically misplaced. It's a constant for LLM request configuration, not for learning tracking. Otherwie, LGTM!
MAX_TOKENS is a configuration for LLM requests, not related to learning tracking. Moving it from LearningTracker class to module level for better semantic placement. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
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.2026-01-21.11-49-16.webm
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.