[tutor] Add AI-Powered Installation Tutor #566
Conversation
Add comprehensive AI tutor for package education with: - Interactive tutoring with Plan→Act→Reflect LangGraph workflow - LLM-powered lessons, code examples, and Q&A (Claude API via LangChain) - SQLite-based progress tracking with topic completion stats - Best practices and step-by-step tutorials - Rich terminal UI with branded output Features: - cortex tutor <package> - Start interactive session - cortex tutor --list - Show studied packages - cortex tutor --progress - View learning progress - cortex tutor --reset - Reset progress Technical: - 266 tests with 85.61% coverage - Lazy imports for non-LLM operations (no API key needed for --list/--progress) - Pydantic models for type safety - 7-layer prompt architecture Closes cortexlinux#131
|
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 new AI-powered "tutor" subsystem under Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as cortex/cli.py
participant TutorCLI as cortex/tutor/cli.py
participant Agent as TutorAgent
participant Tools as LessonLoaderTool/Validators
participant LLM as cortex/tutor/llm.py
participant Storage as SQLiteStore
User->>CLI: cortex tutor docker
CLI->>TutorCLI: cmd_teach(package)
TutorCLI->>Tools: validate_package_name()
TutorCLI->>Agent: teach(package, force_fresh?)
Agent->>Tools: load_lesson_with_fallback(package)
alt cached or fallback
Tools->>Storage: get_cached_lesson()
Storage-->>Tools: lesson
Tools-->>Agent: lesson (source=cache/fallback)
else generate
Agent->>LLM: generate_lesson(package)
LLM-->>Agent: lesson + cost
Agent->>Storage: cache_lesson()
end
Agent-->>TutorCLI: lesson payload
TutorCLI->>Agent: start InteractiveTutor (if interactive)
Agent->>Storage: update_progress()
Storage-->>Agent: ok
Agent-->>User: interactive menu rendered
sequenceDiagram
actor User
participant CLI as cortex/cli.py
participant TutorCLI as cortex/tutor/cli.py
participant Agent as TutorAgent
participant LLM as cortex/tutor/llm.py
participant Storage as SQLiteStore
User->>CLI: cortex tutor docker -q "How to use volumes?"
CLI->>TutorCLI: cmd_question(package, question)
TutorCLI->>Agent: ask(package, question)
Agent->>Storage: get_student_profile()
Agent->>LLM: answer_question(package, question, context)
LLM-->>Agent: answer + cost
Agent->>Storage: (optionally) update_progress()
Agent-->>TutorCLI: answer payload
TutorCLI-->>User: formatted answer
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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 |
CLA Verification PassedAll contributors have signed the CLA.
|
Summary of ChangesHello @srikrishnavansi, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request integrates an advanced AI-powered tutor into Cortex Linux, providing users with an interactive and personalized way to learn about various software packages. The system intelligently generates educational content, tracks learning progress, and offers quick answers to questions, enhancing the user's ability to understand and utilize system tools effectively. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is an excellent and comprehensive pull request that adds a powerful AI Tutor feature. The architecture is well-designed, leveraging modern tools like LangGraph and Pydantic for a robust and maintainable implementation. The code is clean, well-documented, and includes a thorough test suite, which is fantastic to see. The 7-layer prompt architecture is particularly impressive and shows a deep understanding of prompt engineering best practices.
My review focuses on a few minor areas for improvement to enhance maintainability and robustness, such as centralizing CLI argument definitions and removing hardcoded values. Overall, this is a very high-quality contribution.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In @cortex/tutor/cli.py:
- Around line 110-150: The cmd_teach function accepts a fresh flag but never
passes it to the InteractiveTutor; update cmd_teach to forward the fresh flag
when constructing InteractiveTutor (e.g., InteractiveTutor(package,
force_fresh=fresh) or matching param name), and update InteractiveTutor.__init__
in cortex.tutor.agents.tutor_agent to accept and store the corresponding
parameter (force_fresh or fresh) so the interactive session honors the flag.
In @cortex/tutor/config.py:
- Line 44: The db_path field is declared as Path but defaults to None causing a
type mismatch; update the annotation to Optional[Path] (and add from typing
import Optional if missing) for the db_path Field declaration so it accurately
allows None prior to model_post_init, keeping the Field(..., description="Path
to SQLite database") usage and leaving model_post_init behavior unchanged.
In @cortex/tutor/memory/sqlite_store.py:
- Around line 340-377: The singleton profile logic can race when two threads
insert simultaneously; add a UNIQUE constraint to the student_profile table
(e.g., enforce a single row via id PRIMARY KEY or a UNIQUE constant key) and
change _create_default_profile() to perform an atomic upsert (SQLite: INSERT OR
IGNORE or INSERT ... ON CONFLICT DO NOTHING) instead of blindly inserting, then
re-query the table and return the actual stored row; update
get_student_profile() to, after attempting creation, SELECT the profile again
(don’t recurse) so whichever thread succeeded returns the canonical row and
duplicates/constraint errors are avoided.
In @cortex/tutor/tests/test_cli.py:
- Around line 103-115: The test mocks the wrong import location for
InteractiveTutor so cmd_teach still uses the real class; update the patch target
to where cmd_teach looks up the name by replacing
@patch("cortex.tutor.agents.tutor_agent.InteractiveTutor") with
@patch("cortex.tutor.cli.InteractiveTutor") in the test (test_successful_teach)
so the mocked InteractiveTutor instance is injected when cmd_teach("docker")
constructs and calls InteractiveTutor.start.
In @cortex/tutor/tests/test_progress_tracker.py:
- Around line 290-304: The three test stubs in TestConvenienceFunctions
(test_get_learning_progress, test_mark_topic_completed, test_get_package_stats)
should be implemented or removed; to fix, implement them using the temp_db
fixture to create minimal sample data, import and call the actual convenience
functions (get_learning_progress, mark_topic_completed, get_package_stats), then
assert expected behavior: for get_learning_progress verify returned structure
and percentages for a package/topic based on created progress rows, for
mark_topic_completed verify it creates/updates a progress record marking the
topic completed and sets completed_at or equivalent flag, and for
get_package_stats verify aggregate stats (total topics, completed count,
completion rate) match the test data; use the function/class names above to
locate targets and clean up any leftover pass placeholders if you prefer
removing unused tests.
In @cortex/tutor/tools/deterministic/lesson_loader.py:
- Around line 131-149: The clear_cache function's behavior doesn't match its
docstring: clear_cache(package_name) currently overwrites the cache entry via
self.store.cache_lesson(package_name, {}, ttl_hours=0) (marking it
expired/unretrievable to get_cached_lesson) rather than deleting it, and
clear_cache(None) calls self.store.clear_expired_cache() which only removes
already-expired entries. Update the clear_cache docstring to say that passing a
package_name marks that package's cache as expired (using cache_lesson with
ttl_hours=0) and that the function returns 1 on successful marking or 0 on
error; also clarify that calling clear_cache(None) only removes entries whose
expires_at <= now and that the return value is whatever
self.store.clear_expired_cache() returns (number of entries removed). Ensure
references to the symbols clear_cache, self.store.cache_lesson,
self.store.clear_expired_cache, and get_cached_lesson are used in the docstring
to make behavior explicit.
In @docs/AI_TUTOR.md:
- Line 753: The documentation line "Current coverage: **87%** (266 tests)" is
stale; update that exact string to reflect the current test coverage percentage
and test count (replace both the percentage and the test count with the latest
values), and scan for other occurrences of the same string in the docs to keep
coverage numbers consistent; commit the updated line so the README/doc shows the
accurate "Current coverage: **X%** (Y tests)" value.
- Line 6: The coverage badge in the README currently shows 87% ("[](https://github.com/cortexlinux/cortex)"),
which conflicts with the PR's reported 85.61%; update the badge value to match
the actual coverage (replace "87%25" with "85.61%25") or switch to a dynamic
coverage badge that reflects the real-time metric so the percentage stays
accurate.
In @requirements.txt:
- Around line 27-31: The dependency version ranges in requirements (langchain,
langchain-anthropic, langgraph) are too loose and risk pulling breaking major
releases; update the entries for langchain, langchain-anthropic, and langgraph
to use compatible release specifiers (e.g., change langchain>=0.3.0 to an
approximate pin like langchain~=1.2.0, langchain-anthropic>=0.3.0 to
langchain-anthropic~=1.3.0, and langgraph>=0.2.0 to langgraph~=1.0.0) so you
allow patch/minor updates but prevent unintended major version jumps while
leaving pydantic as-is unless you want to similarly tighten it.
🧹 Nitpick comments (42)
cortex/tutor/tools/agentic/examples_provider.py (2)
7-7: Unused import.
Pathfrompathlibis imported but not used in this module.🧹 Suggested fix
-from pathlib import Path from typing import Any, Dict, List, Optional
59-157: Consider reducing duplication between_runand_arun.Both methods share identical prompt construction and response structuring logic. Extracting the common setup into a helper method would improve maintainability.
♻️ Optional refactor to reduce duplication
def _build_chain(self): """Build the prompt-LLM-parser chain.""" prompt = ChatPromptTemplate.from_messages( [ ("system", self._get_system_prompt()), ("human", self._get_generation_prompt()), ] ) return prompt | self.llm | JsonOutputParser() def _build_inputs(self, package_name, topic, difficulty, learning_style, existing_knowledge): """Build input dict for chain invocation.""" return { "package_name": package_name, "topic": topic, "difficulty": difficulty, "learning_style": learning_style, "existing_knowledge": ", ".join(existing_knowledge or []) or "basics", } def _run(self, package_name: str, topic: str, ...) -> Dict[str, Any]: try: chain = self._build_chain() inputs = self._build_inputs(package_name, topic, difficulty, learning_style, existing_knowledge) result = chain.invoke(inputs) examples = self._structure_response(result, package_name, topic) return {"success": True, "examples": examples, "cost_gbp": 0.01} except Exception as e: return {"success": False, "error": str(e), "examples": None}requirements.txt (1)
25-26: Pre-existing duplicate PyYAML entries noted.There are three PyYAML specifications in this file (lines 9, 21, and 25) with conflicting constraints (
>=6.0.0vs==6.0.3). While not introduced by this PR, this could cause dependency resolution issues. Consider consolidating in a follow-up.cortex/tutor/tests/test_branding.py (3)
8-9: Remove unused imports.
MagicMockandStringIOare imported but never used in this test module.Proposed fix
-from unittest.mock import Mock, patch, MagicMock -from io import StringIO +from unittest.mock import Mock, patch
46-70: Consider adding output assertions for stronger tests.These tests accept
capsysbut don't use it to verify the output. While they serve as smoke tests ensuring no exceptions are raised, they don't validate the actual output content.For improved test quality, consider capturing and asserting on key output patterns:
def test_tutor_print_success(self, capsys): tutor_print("Test message", "success") captured = capsys.readouterr() assert "Test message" in captured.out
243-248: Clarify test intent and strengthen assertion.The test name
test_get_user_input_stripssuggests testing strip behavior, but the comment says "get_user_input should return raw input" (no stripping). The assertionassert "spaced" in resultpasses regardless of whether input is stripped or not.Clarify the intended behavior and use a precise assertion:
Proposed fix for raw input (no stripping)
@patch("builtins.input", return_value=" spaced ") - def test_get_user_input_strips(self, mock_input): - """Test input stripping is not done (raw input).""" + def test_get_user_input_returns_raw(self, mock_input): + """Test get_user_input returns raw input without stripping.""" result = get_user_input("Enter value") - # Note: get_user_input should return raw input - assert "spaced" in result + assert result == " spaced "cortex/tutor/tests/test_tutor_agent.py (3)
9-10: Remove unused imports.
tempfileandPathare imported but never used in this test module.Proposed fix
-import tempfile -from pathlib import Path
78-84: Floating-point comparison may be fragile.Direct equality comparison
state["cost_gbp"] == 0.03on floats can fail due to precision issues. While0.02 + 0.01is exact in this case, consider usingpytest.approxfor robustness:assert state["cost_gbp"] == pytest.approx(0.03)
303-317: Consider splitting multiple exception assertions into separate tests.This test validates two different input validation scenarios (empty package name and empty question) in a single test. If the first assertion fails, the second is never executed, potentially hiding additional issues.
Consider splitting into
test_ask_validates_package_nameandtest_ask_validates_questionfor better test isolation.cortex/tutor/tests/test_interactive_tutor.py (2)
8-8: Remove unused import.
callis imported fromunittest.mockbut never used in this test module.Proposed fix
-from unittest.mock import Mock, patch, MagicMock, call +from unittest.mock import Mock, patch, MagicMock
246-262: Consider adding behavior assertions for None lesson handling.The test verifies that methods don't raise exceptions when
lessonisNone, which is valuable. However, adding assertions about the expected behavior (e.g., verifying that appropriate "No content" messages are printed or that methods return early) would strengthen the test.cortex/tutor/prompts/tools/lesson_generator.md (2)
66-74: Add language identifier to fenced code block.Per static analysis (markdownlint MD040), this code block should specify a language. Since it's JSON, add
jsonafter the opening fence.-``` +```json { "package_name": "name of the package to teach",
104-132: Consider adding a language identifier for the workflow code block.This fenced code block (starting at Line 104) lacks a language specification. Since it's pseudo-code/plaintext describing the workflow, you could use
textorplaintextas the identifier to satisfy linting rules.cortex/tutor/tests/test_integration.py (1)
207-216: Consider adding minimal assertions or removing unusedcapsys.The
capsysfixture is captured but not used in the assertions. If verifying Rich console output is impractical, consider removing the capture or adding a comment explaining the intentional no-op check.♻️ Suggested refactor
def test_tutor_print_success(self, capsys): """Test tutor_print with success status.""" tutor_print("Test message", "success") - captured = capsys.readouterr() - # Rich console output is complex, just ensure no errors + # Rich console output is complex; this test verifies no exceptions are raised def test_tutor_print_error(self, capsys): """Test tutor_print with error status.""" tutor_print("Error message", "error") - captured = capsys.readouterr() + # Verifies no exceptions are raised with error statuscortex/tutor/tests/test_validators.py (2)
216-219: Type hint inconsistency:sanitize_inputacceptsNonebut typed asstr.The test correctly verifies that
sanitize_input(None)returns"", but the function signature invalidators.pyis typed assanitize_input(input_text: str). Consider updating the type hint toOptional[str]for consistency, or document this behavior.
240-243: Same type consideration forextract_package_name(None).Similar to
sanitize_input, this testsNoneinput while the function signature expectsstr.cortex/tutor/tests/test_deterministic_tools.py (1)
99-108: Consider usingtmp_pathfixture instead of customtemp_db.Pytest provides a built-in
tmp_pathfixture that handles cleanup automatically. The current approach works, but usingtmp_pathwould be slightly more idiomatic.♻️ Optional refactor
- @pytest.fixture - def temp_db(self): - """Create a temporary database.""" - with tempfile.TemporaryDirectory() as tmpdir: - yield Path(tmpdir) / "test.db" + @pytest.fixture + def temp_db(self, tmp_path): + """Create a temporary database.""" + return tmp_path / "test.db"cortex/tutor/tools/__init__.py (1)
7-11: Consider addingLessonLoaderToolto parent package exports for API consistency.The
cortex/tutor/tools/deterministic/__init__.pyexportsLessonLoaderTool, but this parent package's__all__does not include it. Since other deterministic tools (ProgressTrackerTool,validate_package_name,validate_input) are re-exported here,LessonLoaderToolshould also be added for a complete and consistent public API. This allows users to import it asfrom cortex.tutor.tools import LessonLoaderToolalongside other deterministic tools.cortex/tutor/tests/test_progress_tracker.py (1)
27-44: Consider adding return type hints to fixtures.Per coding guidelines, type hints are required. The fixtures should specify return types.
♻️ Suggested improvement
@pytest.fixture -def temp_db(): +def temp_db() -> Path: """Create a temporary database for testing.""" with tempfile.TemporaryDirectory() as tmpdir: db_path = Path(tmpdir) / "test_progress.db" yield db_path @pytest.fixture -def store(temp_db): +def store(temp_db: Path) -> SQLiteStore: """Create a SQLite store with temp database.""" return SQLiteStore(temp_db) @pytest.fixture -def tracker(temp_db): +def tracker(temp_db: Path) -> ProgressTrackerTool: """Create a progress tracker with temp database.""" return ProgressTrackerTool(temp_db)cortex/tutor/tests/test_agent_methods.py (1)
7-10: Remove unused imports.
tempfileandPathare imported but never used in this test file.♻️ Proposed fix
import pytest from unittest.mock import Mock, patch, MagicMock -import tempfile -from pathlib import Pathcortex/tutor/tests/test_cli.py (1)
10-10: Remove unused import.
StringIOis imported but never used in this file.♻️ Proposed fix
-from io import StringIOcortex/tutor/tests/test_tools.py (2)
8-8: Remove unused import.
MagicMockis imported but never used; onlyMockis used.♻️ Proposed fix
-from unittest.mock import Mock, patch, MagicMock +from unittest.mock import Mock, patch
276-307: Fragile test pattern using__new__to bypass__init__.Using
ConversationHandler.__new__()and manually setting attributes bypasses the constructor, which can lead to tests that don't catch initialization bugs. Consider properly mocking the config dependency and instantiating normally.♻️ Suggested improvement
def test_build_context_empty(self): """Test context building with empty history.""" - with patch("cortex.tutor.tools.agentic.qa_handler.get_config"): - handler = ConversationHandler.__new__(ConversationHandler) - handler.history = [] + with patch("cortex.tutor.tools.agentic.qa_handler.get_config") as mock_config: + mock_config.return_value = Mock( + anthropic_api_key="test_key", + model="claude-sonnet-4-20250514", + ) + with patch("cortex.tutor.tools.agentic.qa_handler.ChatAnthropic"): + handler = ConversationHandler() + handler.history = [] # Reset for test context = handler._build_context() assert "Starting fresh" in contextcortex/tutor/agents/tutor_agent/tutor_agent.py (2)
248-258: Unusedcurrent_stepattribute and missingforce_freshsupport.The
current_stepattribute (line 258) is initialized but never used in the class. Additionally, the CLI's--freshflag (passed tocmd_teach) is not propagated toInteractiveTutor, so users cannot force fresh content generation in interactive mode.Consider either:
- Removing
current_stepif not needed- Adding
force_fresh: bool = Falseparameter to__init__and passing it toself.agent.teach()♻️ Suggested fix to support force_fresh
- def __init__(self, package_name: str) -> None: + def __init__(self, package_name: str, force_fresh: bool = False) -> None: """ Initialize interactive tutor for a package. Args: package_name: Package to learn. + force_fresh: Skip cache and generate fresh content. """ self.package_name = package_name self.agent = TutorAgent(verbose=False) self.lesson: Optional[Dict[str, Any]] = None - self.current_step = 0 + self.force_fresh = force_fresh
439-446: Consider extracting the magic number5as a constant.The default topic count of
5(line 443) is used as a fallback whenstats.get("total", 0)is0. While the inline comment explains it, extracting this as a named constant would improve clarity.cortex/tutor/tools/deterministic/lesson_loader.py (1)
100-106: Consider async SQLite operations in the future.The async method currently delegates to the synchronous implementation. This is acceptable for fast SQLite operations but could be improved with
aiosqliteif async performance becomes important.cortex/tutor/tools/agentic/lesson_generator.py (3)
1-19: Unused imports fromlesson_context.
LessonContext,CodeExample, andTutorialStep(line 18) are imported but not used in this module. The_structure_responsemethod returns a plainDictrather than using these Pydantic models for validation.Consider either:
- Removing unused imports
- Using
LessonContext.model_validate()in_structure_responseto leverage Pydantic validation
21-36:_load_prompt_templateis defined but never called.The function loads a prompt template from a file but is never invoked anywhere in this module. The actual prompts are generated inline by
_get_system_prompt()and_get_generation_prompt().♻️ Consider removing unused function
-# Load prompt template -def _load_prompt_template() -> str: - """Load the lesson generator prompt from file.""" - prompt_path = Path(__file__).parent.parent.parent / "prompts" / "tools" / "lesson_generator.md" - if prompt_path.exists(): - return prompt_path.read_text() - # Fallback inline prompt - return """You are a lesson content generator. Generate comprehensive educational content - for the package: {package_name} - - Student level: {student_level} - Learning style: {learning_style} - Focus areas: {focus_areas} - - Return a JSON object with: summary, explanation, use_cases, best_practices, - code_examples, tutorial_steps, installation_command, confidence.""" - -
142-184: Consider extracting shared prompt construction logic.The
_arunmethod duplicates the prompt construction from_run(lines 105-110 vs 152-157). Extracting this to a helper method would reduce duplication.cortex/tutor/contracts/progress_context.py (1)
98-100:datetime.utcnow()is deprecated; use timezone-aware datetime.
datetime.utcnow()is deprecated since Python 3.12. Usedatetime.now(timezone.utc)for timezone-aware UTC timestamps.♻️ Suggested fix
+from datetime import datetime, timezone -from datetime import datetime ... last_updated: datetime = Field( - default_factory=datetime.utcnow, description="Last update timestamp" + default_factory=lambda: datetime.now(timezone.utc), description="Last update timestamp (UTC)" )cortex/tutor/agents/tutor_agent/graph.py (1)
1-25: Unused import:ExamplesProviderTool.
ExamplesProviderTool(line 24) is imported but never used in this module.♻️ Remove unused import
from cortex.tutor.tools.agentic.qa_handler import QAHandlerTool -from cortex.tutor.tools.agentic.examples_provider import ExamplesProviderToolcortex/tutor/contracts/lesson_context.py (1)
99-101:datetime.utcnow()is deprecated.Same issue as in
progress_context.py. Usedatetime.now(timezone.utc)for timezone-aware UTC timestamps.cortex/tutor/tools/deterministic/validators.py (1)
40-49: Unused constant:KNOWN_PACKAGE_CATEGORIES.This constant is defined but never referenced anywhere in the module. Consider removing it or documenting its intended future use.
🔧 Suggested fix
Either remove the unused constant:
-# Common package categories for validation hints -KNOWN_PACKAGE_CATEGORIES = [ - "system", # apt, systemctl, journalctl - "development", # git, docker, npm, pip - "database", # postgresql, mysql, redis, mongodb - "web", # nginx, apache, curl, wget - "security", # ufw, fail2ban, openssl - "networking", # ssh, netstat, iptables - "utilities", # vim, tmux, htop, grep -]Or add a TODO comment explaining its intended use:
+# TODO: Use for package categorization hints in future validation enhancements KNOWN_PACKAGE_CATEGORIES = [cortex/tutor/tools/deterministic/progress_tracker.py (4)
7-10: Unused import:Path.The
Pathtype is imported but never used in this module.🔧 Suggested fix
from datetime import datetime -from pathlib import Path from typing import Any, Dict, List, Optional
115-117: Async method delegates to sync - no actual async benefit.The
_arunmethod simply calls the synchronous_run, which blocks the event loop. This is acceptable if the underlying SQLite operations are fast, but consider noting this limitation in the docstring.📝 Suggested docstring improvement
async def _arun(self, *args: Any, **kwargs: Any) -> Dict[str, Any]: - """Async version - delegates to sync implementation.""" + """Async version - delegates to sync implementation. + + Note: SQLite operations are synchronous. For true async, consider + aiosqlite in future iterations. + """ return self._run(*args, **kwargs)
258-280: Potential parameter shadowing issue.The
conceptparameter is fetched fromkwargsfirst, then falls back to the function parameter. This is redundant sinceconceptwould already be inkwargsif passed as a keyword argument.🔧 Suggested simplification
def _add_mastered_concept( self, concept: Optional[str] = None, **kwargs: Any, ) -> Dict[str, Any]: """Add a mastered concept to student profile.""" - concept = kwargs.get("concept") or concept if not concept: return {"success": False, "error": "concept required"} self.store.add_mastered_concept(concept) return {"success": True, "message": f"Added mastered concept: {concept}"} def _add_weak_concept( self, concept: Optional[str] = None, **kwargs: Any, ) -> Dict[str, Any]: """Add a weak concept to student profile.""" - concept = kwargs.get("concept") or concept if not concept: return {"success": False, "error": "concept required"} self.store.add_weak_concept(concept) return {"success": True, "message": f"Added weak concept: {concept}"}
301-346: Convenience functions create new tool instances on each call.Each call instantiates a new
ProgressTrackerTool, which opens a new database connection. For repeated operations, this is inefficient. Consider documenting this trade-off or providing a module-level singleton option.📝 Suggested improvement
Add a note about usage patterns:
# Convenience functions for direct usage +# Note: Each function creates a new tool instance. For batch operations, +# instantiate ProgressTrackerTool once and reuse it. def get_learning_progress(package_name: str, topic: str) -> Optional[Dict[str, Any]]:Or provide a singleton pattern:
_default_tool: Optional[ProgressTrackerTool] = None def _get_tool() -> ProgressTrackerTool: global _default_tool if _default_tool is None: _default_tool = ProgressTrackerTool() return _default_toolcortex/tutor/tools/agentic/qa_handler.py (4)
7-8: Unused import:Path.The
Pathtype is imported but never used in this module.🔧 Suggested fix
-from pathlib import Path from typing import Any, Dict, List, Optional
59-162: Significant code duplication between_runand_arun.The sync and async methods share nearly identical logic. Extract the common setup into a helper method.
♻️ Suggested refactor
+ def _build_chain(self): + """Build the prompt chain for Q&A.""" + prompt = ChatPromptTemplate.from_messages( + [ + ("system", self._get_system_prompt()), + ("human", self._get_qa_prompt()), + ] + ) + return prompt | self.llm | JsonOutputParser() + + def _build_invoke_params( + self, + package_name: str, + question: str, + learning_style: str, + mastered_concepts: Optional[List[str]], + weak_concepts: Optional[List[str]], + lesson_context: Optional[str], + ) -> Dict[str, Any]: + """Build parameters for chain invocation.""" + return { + "package_name": package_name, + "question": question, + "learning_style": learning_style, + "mastered_concepts": ", ".join(mastered_concepts or []) or "none specified", + "weak_concepts": ", ".join(weak_concepts or []) or "none specified", + "lesson_context": lesson_context or "starting fresh", + } + def _run(self, ...) -> Dict[str, Any]: try: - prompt = ChatPromptTemplate.from_messages(...) - chain = prompt | self.llm | JsonOutputParser() - result = chain.invoke({...}) + chain = self._build_chain() + params = self._build_invoke_params(...) + result = chain.invoke(params) # ... rest unchanged
105-109: Hardcoded cost estimate may become inaccurate.The
cost_gbp: 0.02is static and may not reflect actual API costs as pricing changes.📝 Suggested improvement
Consider computing based on token usage or making it configurable:
+ # Note: Estimated cost; actual varies by token usage return { "success": True, "answer": answer, - "cost_gbp": 0.02, + "cost_estimate_gbp": 0.02, # Approximate based on ~2K tokens }
268-344: Unbounded conversation history growth.The
historylist grows indefinitely during long sessions. While_build_contextonly uses the last 3 exchanges, the full history is retained in memory.🔧 Suggested fix to cap history size
def ask( self, question: str, ... ) -> Dict[str, Any]: ... # Update history if result.get("success"): self.history.append( { "question": question, "answer": result["answer"].get("answer", ""), } ) + # Keep history bounded to prevent memory growth + max_history = 20 # or make configurable + if len(self.history) > max_history: + self.history = self.history[-max_history:] return resultcortex/tutor/memory/sqlite_store.py (1)
41-48: Replace mutable default values withField(default_factory=list).Pydantic recommends using
Field(default_factory=list)for list defaults to ensure each instance gets a fresh list and to maintain consistency across Pydantic versions. While Pydantic does deepcopy mutable defaults, explicitly usingdefault_factoryis the clearer, future-proof approach.🔧 Suggested fix
+from pydantic import BaseModel, Field -from pydantic import BaseModel class StudentProfile(BaseModel): """Model for student profile.""" id: Optional[int] = None - mastered_concepts: List[str] = [] - weak_concepts: List[str] = [] + mastered_concepts: List[str] = Field(default_factory=list) + weak_concepts: List[str] = Field(default_factory=list) learning_style: str = "reading" # visual, reading, hands-on last_session: Optional[str] = None
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (42)
cortex/cli.pycortex/tutor/__init__.pycortex/tutor/agents/__init__.pycortex/tutor/agents/tutor_agent/__init__.pycortex/tutor/agents/tutor_agent/graph.pycortex/tutor/agents/tutor_agent/state.pycortex/tutor/agents/tutor_agent/tutor_agent.pycortex/tutor/branding.pycortex/tutor/cli.pycortex/tutor/config.pycortex/tutor/contracts/__init__.pycortex/tutor/contracts/lesson_context.pycortex/tutor/contracts/progress_context.pycortex/tutor/memory/__init__.pycortex/tutor/memory/sqlite_store.pycortex/tutor/prompts/agents/tutor/system.mdcortex/tutor/prompts/tools/examples_provider.mdcortex/tutor/prompts/tools/lesson_generator.mdcortex/tutor/prompts/tools/qa_handler.mdcortex/tutor/tests/__init__.pycortex/tutor/tests/test_agent_methods.pycortex/tutor/tests/test_agentic_tools.pycortex/tutor/tests/test_branding.pycortex/tutor/tests/test_cli.pycortex/tutor/tests/test_deterministic_tools.pycortex/tutor/tests/test_integration.pycortex/tutor/tests/test_interactive_tutor.pycortex/tutor/tests/test_progress_tracker.pycortex/tutor/tests/test_tools.pycortex/tutor/tests/test_tutor_agent.pycortex/tutor/tests/test_validators.pycortex/tutor/tools/__init__.pycortex/tutor/tools/agentic/__init__.pycortex/tutor/tools/agentic/examples_provider.pycortex/tutor/tools/agentic/lesson_generator.pycortex/tutor/tools/agentic/qa_handler.pycortex/tutor/tools/deterministic/__init__.pycortex/tutor/tools/deterministic/lesson_loader.pycortex/tutor/tools/deterministic/progress_tracker.pycortex/tutor/tools/deterministic/validators.pydocs/AI_TUTOR.mdrequirements.txt
🧰 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/tutor/agents/__init__.pycortex/tutor/agents/tutor_agent/state.pycortex/tutor/agents/tutor_agent/__init__.pycortex/tutor/tests/__init__.pycortex/tutor/memory/__init__.pycortex/tutor/tools/agentic/__init__.pycortex/tutor/tests/test_branding.pycortex/tutor/tests/test_validators.pycortex/tutor/tests/test_agent_methods.pycortex/tutor/tests/test_cli.pycortex/tutor/config.pycortex/tutor/tools/__init__.pycortex/tutor/tests/test_tools.pycortex/tutor/tools/deterministic/__init__.pycortex/cli.pycortex/tutor/tests/test_tutor_agent.pycortex/tutor/cli.pycortex/tutor/tests/test_interactive_tutor.pycortex/tutor/tests/test_progress_tracker.pycortex/tutor/contracts/progress_context.pycortex/tutor/contracts/lesson_context.pycortex/tutor/tests/test_agentic_tools.pycortex/tutor/tests/test_integration.pycortex/tutor/branding.pycortex/tutor/agents/tutor_agent/graph.pycortex/tutor/tools/agentic/examples_provider.pycortex/tutor/tools/agentic/qa_handler.pycortex/tutor/tools/deterministic/lesson_loader.pycortex/tutor/agents/tutor_agent/tutor_agent.pycortex/tutor/tools/deterministic/progress_tracker.pycortex/tutor/tests/test_deterministic_tools.pycortex/tutor/contracts/__init__.pycortex/tutor/__init__.pycortex/tutor/tools/agentic/lesson_generator.pycortex/tutor/tools/deterministic/validators.pycortex/tutor/memory/sqlite_store.py
{setup.py,setup.cfg,pyproject.toml,**/__init__.py}
📄 CodeRabbit inference engine (AGENTS.md)
Use Python 3.10 or higher as the minimum supported version
Files:
cortex/tutor/agents/__init__.pycortex/tutor/agents/tutor_agent/__init__.pycortex/tutor/tests/__init__.pycortex/tutor/memory/__init__.pycortex/tutor/tools/agentic/__init__.pycortex/tutor/tools/__init__.pycortex/tutor/tools/deterministic/__init__.pycortex/tutor/contracts/__init__.pycortex/tutor/__init__.py
🧠 Learnings (2)
📚 Learning: 2025-12-11T12:03:24.071Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T12:03:24.071Z
Learning: Applies to **/*.py : Type hints required in Python code
Applied to files:
requirements.txt
📚 Learning: 2025-12-11T12:03:24.071Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T12:03:24.071Z
Learning: Applies to tests/**/*.py : Maintain >80% test coverage for pull requests
Applied to files:
cortex/tutor/tests/__init__.py
🧬 Code graph analysis (20)
cortex/tutor/agents/__init__.py (2)
cortex/cli.py (1)
tutor(1016-1074)cortex/tutor/agents/tutor_agent/tutor_agent.py (1)
TutorAgent(20-238)
cortex/tutor/agents/tutor_agent/__init__.py (2)
cortex/tutor/agents/tutor_agent/state.py (1)
TutorAgentState(50-104)cortex/tutor/agents/tutor_agent/tutor_agent.py (2)
TutorAgent(20-238)InteractiveTutor(241-447)
cortex/tutor/memory/__init__.py (1)
cortex/tutor/memory/sqlite_store.py (1)
SQLiteStore(51-531)
cortex/tutor/tools/agentic/__init__.py (3)
cortex/tutor/tools/agentic/lesson_generator.py (1)
LessonGeneratorTool(39-303)cortex/tutor/tools/agentic/examples_provider.py (1)
ExamplesProviderTool(19-236)cortex/tutor/tools/agentic/qa_handler.py (1)
QAHandlerTool(19-241)
cortex/tutor/tests/test_validators.py (1)
cortex/tutor/tools/deterministic/validators.py (11)
validate_package_name(52-94)validate_input(97-136)validate_question(139-149)validate_topic(152-174)validate_score(177-193)validate_learning_style(196-211)sanitize_input(214-237)extract_package_name(240-278)get_validation_errors(281-321)validate_all(349-368)ValidationResult(324-346)
cortex/tutor/tests/test_agent_methods.py (2)
cortex/tutor/agents/tutor_agent/graph.py (10)
plan_node(30-87)load_cache_node(90-109)generate_lesson_node(112-154)qa_node(157-199)reflect_node(202-251)fail_node(254-275)route_after_plan(281-297)route_after_act(300-311)create_tutor_graph(317-373)get_tutor_graph(380-390)cortex/tutor/agents/tutor_agent/tutor_agent.py (8)
TutorAgent(20-238)teach(47-94)ask(96-139)get_profile(155-162)update_learning_style(164-175)mark_completed(177-195)reset_progress(197-208)get_packages_studied(210-218)
cortex/tutor/tests/test_cli.py (1)
cortex/tutor/cli.py (7)
create_parser(33-107)cmd_teach(110-150)cmd_question(153-208)cmd_list_packages(211-238)cmd_progress(241-311)cmd_reset(314-345)main(348-386)
cortex/tutor/config.py (6)
cortex/tutor/tools/agentic/examples_provider.py (1)
Config(39-40)cortex/tutor/tools/agentic/lesson_generator.py (1)
Config(62-63)cortex/tutor/tools/agentic/qa_handler.py (1)
Config(39-40)cortex/tutor/tools/deterministic/lesson_loader.py (1)
Config(35-36)cortex/tutor/tools/deterministic/progress_tracker.py (1)
Config(45-46)cortex/logging_system.py (1)
debug(196-198)
cortex/tutor/tools/__init__.py (5)
cortex/tutor/tools/deterministic/progress_tracker.py (1)
ProgressTrackerTool(23-295)cortex/tutor/tools/deterministic/validators.py (2)
validate_package_name(52-94)validate_input(97-136)cortex/tutor/tools/agentic/lesson_generator.py (1)
LessonGeneratorTool(39-303)cortex/tutor/tools/agentic/examples_provider.py (1)
ExamplesProviderTool(19-236)cortex/tutor/tools/agentic/qa_handler.py (1)
QAHandlerTool(19-241)
cortex/tutor/tests/test_tools.py (1)
cortex/tutor/tools/deterministic/lesson_loader.py (4)
LessonLoaderTool(18-149)get_fallback_lesson(227-237)load_lesson_with_fallback(240-276)_run(51-98)
cortex/tutor/tools/deterministic/__init__.py (3)
cortex/tutor/tools/deterministic/progress_tracker.py (1)
ProgressTrackerTool(23-295)cortex/tutor/tools/deterministic/validators.py (2)
validate_package_name(52-94)validate_input(97-136)cortex/tutor/tools/deterministic/lesson_loader.py (1)
LessonLoaderTool(18-149)
cortex/tutor/tests/test_tutor_agent.py (3)
cortex/tutor/tools/agentic/qa_handler.py (1)
_run(59-116)cortex/tutor/tools/deterministic/lesson_loader.py (1)
_run(51-98)cortex/tutor/config.py (1)
reset_config(144-147)
cortex/tutor/cli.py (5)
cortex/tutor/branding.py (8)
print_banner(53-69)tutor_print(35-50)print_table(162-180)print_progress_summary(143-159)print_error_panel(238-251)print_success_panel(254-267)get_user_input(194-216)print_code_example(102-115)cortex/tutor/tools/deterministic/validators.py (1)
validate_package_name(52-94)cortex/tutor/config.py (3)
Config(19-121)from_env(60-89)get_db_path(101-109)cortex/tutor/memory/sqlite_store.py (5)
SQLiteStore(51-531)get_packages_studied(520-531)get_completion_stats(257-286)get_all_progress(171-203)reset_progress(500-518)cortex/tutor/agents/tutor_agent/tutor_agent.py (6)
InteractiveTutor(241-447)start(260-326)TutorAgent(20-238)ask(96-139)get_packages_studied(210-218)reset_progress(197-208)
cortex/tutor/tests/test_interactive_tutor.py (2)
cortex/tutor/tools/agentic/qa_handler.py (1)
ask(286-327)cortex/tutor/memory/sqlite_store.py (1)
get_progress(142-169)
cortex/tutor/tests/test_agentic_tools.py (3)
cortex/tutor/tools/agentic/lesson_generator.py (2)
LessonGeneratorTool(39-303)_structure_response(251-303)cortex/tutor/tools/agentic/examples_provider.py (1)
_structure_response(208-236)cortex/tutor/tools/agentic/qa_handler.py (5)
_structure_response(215-241)QAHandlerTool(19-241)ConversationHandler(268-344)_build_context(329-340)clear_history(342-344)
cortex/tutor/tests/test_integration.py (5)
cortex/tutor/config.py (5)
Config(19-121)get_config(128-141)reset_config(144-147)from_env(60-89)ensure_data_dir(91-99)cortex/tutor/branding.py (2)
tutor_print(35-50)print_banner(53-69)cortex/tutor/contracts/lesson_context.py (6)
LessonContext(36-132)CodeExample(13-21)TutorialStep(24-33)to_json(103-105)from_json(108-110)to_display_dict(120-132)cortex/tutor/contracts/progress_context.py (8)
ProgressContext(66-143)PackageProgress(23-63)TopicProgress(13-20)completion_percentage(33-38)average_score(42-46)is_complete(61-63)get_next_topic(54-59)get_recommendations(115-131)cortex/tutor/cli.py (1)
create_parser(33-107)
cortex/tutor/contracts/__init__.py (2)
cortex/tutor/contracts/lesson_context.py (1)
LessonContext(36-132)cortex/tutor/contracts/progress_context.py (1)
ProgressContext(66-143)
cortex/tutor/__init__.py (2)
cortex/tutor/config.py (1)
Config(19-121)cortex/tutor/branding.py (1)
tutor_print(35-50)
cortex/tutor/tools/agentic/lesson_generator.py (2)
cortex/tutor/config.py (2)
get_config(128-141)Config(19-121)cortex/tutor/contracts/lesson_context.py (3)
LessonContext(36-132)CodeExample(13-21)TutorialStep(24-33)
cortex/tutor/memory/sqlite_store.py (5)
cortex/tutor/contracts/progress_context.py (1)
total_time_seconds(50-52)cortex/tutor/agents/tutor_agent/tutor_agent.py (3)
get_progress(141-153)reset_progress(197-208)get_packages_studied(210-218)cortex/tutor/tools/deterministic/progress_tracker.py (1)
mark_topic_completed(317-331)cortex/hwprofiler.py (1)
profile(421-448)cortex/tutor/tools/deterministic/lesson_loader.py (1)
cache_lesson(108-129)
🪛 markdownlint-cli2 (0.18.1)
docs/AI_TUTOR.md
92-92: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
250-250: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
310-310: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
362-362: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
401-401: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
443-443: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
cortex/tutor/prompts/tools/lesson_generator.md
66-66: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
97-97: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
|
recheck |
Anshgrover23
left a comment
Anshgrover23
left a comment
There was a problem hiding this comment.
@srikrishnavansi Follow the contributing.md guidelines( i.e. add demonstrating video in pr description, add Documentation for the feature you're creating, add tests and AI Usage as well, thanks.
Also, kindly address CODERABBITAI comments.
|
I will update the PR and changes suggested by CodeRabbit. |
srikrishnavansi
changed the title
Fixes identified in PR cortexlinux#566 code review: - Fix coverage badge discrepancy (87% → 85.6%) in docs/AI_TUTOR.md - Fix db_path type annotation to Optional[Path] in config.py - Remove unused ExamplesProviderTool import from graph.py - Add DEFAULT_TUTOR_TOPICS constant to avoid magic numbers - Fix --fresh flag propagation to InteractiveTutor - Fix race condition in profile creation with INSERT OR IGNORE - Tighten version constraints in requirements.txt (<2.0.0 bounds) - Update clear_cache docstring to match actual behavior - Implement empty test methods in TestConvenienceFunctions All 266 tests pass with 86% coverage.
…/cortex into feature/ai-tutor
- Fix import sorting (I001) with ruff format - Modernize type annotations for Python 3.10+: - Replace Dict/List/Tuple with dict/list/tuple - Replace Optional[X] with X | None - Remove deprecated typing imports - Fix f-string without placeholders All 266 tests pass.
…/cortex into feature/ai-tutor
The tutor tests were in cortex/tutor/tests/ but CI runs pytest tests/ - causing 0% coverage on tutor code. Copied tests to tests/tutor/ so CI discovers them and includes tutor code in coverage metrics. All 266 tutor tests pass from new location.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @cortex/tutor/contracts/lesson_context.py:
- Around line 97-99: The Field default_factory for generated_at currently uses
datetime.utcnow (in the generated_at: datetime = Field(...
default_factory=datetime.utcnow ...)), which is deprecated; replace it with a
timezone-aware factory using datetime.now(timezone.utc) and ensure timezone is
imported from datetime (or use datetime.timezone) so generated_at remains
UTC-aware; update the default_factory reference in the generated_at Field and
add the necessary import for timezone at the top of the module.
In @cortex/tutor/contracts/progress_context.py:
- Line 157: The QuizContext.timestamp Field currently uses datetime.utcnow() (in
the default_factory) which is deprecated; change the default_factory for
QuizContext.timestamp to produce a timezone-aware UTC datetime (e.g., use
datetime.now(timezone.utc) or an equivalent lambda), and update imports if
needed to include timezone so the Field default is timezone-aware while keeping
the type as datetime.
- Around line 98-100: The Field default_factory for last_updated in
progress_context.py uses deprecated datetime.utcnow(); update the
default_factory to use datetime.now(timezone.utc) instead and ensure timezone is
imported from datetime (mirror the fix applied in lesson_context.py); modify the
default_factory on the last_updated Field (and any similar uses in this module)
to call datetime.now(timezone.utc) to produce an aware UTC timestamp.
In @cortex/tutor/memory/sqlite_store.py:
- Around line 206-239: The upsert_progress method currently returns
cursor.lastrowid which is 0 when the INSERT triggers the ON CONFLICT ... DO
UPDATE branch; change upsert_progress (learning_progress upsert) to detect that
case and return the actual row id by querying the table for the record after the
upsert (e.g., SELECT id or rowid WHERE package_name = ? AND topic = ?) when
cursor.lastrowid is 0, ensuring the function always returns the real row id for
the given progress.package_name and progress.topic.
- Around line 419-444: The functions add_mastered_concept and add_weak_concept
perform a read-modify-write via get_student_profile() and
update_student_profile(), which opens a TOCTOU race; replace the Python-side
mutation with an atomic database operation (or wrap the operation in a single
transaction) so concurrent updates cannot be lost: implement a single SQL UPDATE
that uses JSON functions to append/remove the concept from the mastered_concepts
or weak_concepts JSON array (and remove it from the opposite array when
promoting/demoting), or execute the read and write inside a DB transaction with
appropriate locking/isolation in the same code paths where get_student_profile()
and update_student_profile() are called so add_mastered_concept and
add_weak_concept no longer rely on separate read and write calls that can
interleave.
🧹 Nitpick comments (46)
cortex/tutor/tests/test_validators.py (2)
7-7: Unusedpytestimport.The
pytestmodule is imported but never used in this test file. Nopytest.fixture,pytest.raises, or other pytest-specific features are utilized.🔧 Suggested fix
-import pytest - from cortex.tutor.tools.deterministic.validators import (
165-180: Consider adding boundary value tests for score validation.The tests cover valid scores and out-of-range values, but consider adding explicit boundary tests for exactly
0.0and1.0to ensure inclusive boundary handling is verified.🧪 Additional boundary test
def test_boundary_scores(self): """Test exact boundary values are valid.""" # Ensure 0.0 and 1.0 are explicitly accepted is_valid, error = validate_score(0.0) assert is_valid, "0.0 should be valid" is_valid, error = validate_score(1.0) assert is_valid, "1.0 should be valid"cortex/tutor/memory/sqlite_store.py (2)
52-57: Docstring claims "connection pooling" but implementation creates new connections.The class docstring mentions "connection pooling," but
_get_connectioncreates a new connection on each call and closes it after use. This is connection-per-request, not pooling.📝 Suggested docstring fix
class SQLiteStore: """ SQLite-based storage for learning progress and student data. - Thread-safe implementation with connection pooling. + Thread-safe implementation with per-request connections. Attributes: db_path: Path to the SQLite database file. """
42-50: UseField(default_factory=list)for explicit mutable defaults.While Pydantic v2 safely deep-copies empty list defaults for each instance, the idiomatic approach is
Field(default_factory=list)to explicitly signal that each instance gets its own list. This also guards against issues if the codebase ever migrates to older Pydantic versions where mutable defaults were not safely handled. Apply tomastered_conceptsandweak_conceptsfields.cortex/tutor/tests/test_deterministic_tools.py (2)
9-9: Unused imports fromunittest.mock.
Mockandpatchare imported but never used in this test file.🔧 Suggested fix
import tempfile from pathlib import Path -from unittest.mock import Mock, patch import pytest
100-109: Duplicatetemp_dbfixture definition.The
temp_dbfixture is defined identically in bothTestLessonLoaderTool(lines 26-30) andTestProgressTrackerTool(lines 100-104). Consider moving it to aconftest.pyfile for reuse.♻️ Suggested refactor
Create
cortex/tutor/tests/conftest.py:import tempfile from pathlib import Path import pytest @pytest.fixture def temp_db(): """Create a temporary database.""" with tempfile.TemporaryDirectory() as tmpdir: yield Path(tmpdir) / "test.db"Then remove the duplicate fixtures from both test classes.
tests/tutor/test_agentic_tools.py (1)
7-7: Remove unused importMagicMock.
MagicMockis imported but never used in this file; onlyMockis utilized.Suggested fix
-from unittest.mock import MagicMock, Mock, patch +from unittest.mock import Mock, patchcortex/tutor/tests/test_agent_methods.py (1)
7-11: Remove unused imports.
tempfileandMagicMockare imported but not used in this file.Suggested fix
-import tempfile from pathlib import Path -from unittest.mock import MagicMock, Mock, patch +from unittest.mock import Mock, patchcortex/tutor/tests/test_branding.py (2)
7-10: Remove unused imports.
StringIOandMagicMockare imported but never used.Suggested fix
-from io import StringIO -from unittest.mock import MagicMock, Mock, patch +from unittest.mock import Mock, patch
244-249: Clarify test assertion for stripping behavior.The test name suggests verifying stripping behavior, but
assert "spaced" in resultpasses whetherresultis" spaced "or"spaced". Consider asserting the exact expected value.Suggested fix for explicit verification
@patch("builtins.input", return_value=" spaced ") def test_get_user_input_strips(self, mock_input): """Test input stripping is not done (raw input).""" result = get_user_input("Enter value") - # Note: get_user_input should return raw input - assert "spaced" in result + # Verify raw input is returned without stripping + assert result == " spaced "tests/tutor/test_tutor_agent.py (1)
7-11: Remove unused imports.
tempfileandMagicMockare imported but not used.Suggested fix
-import tempfile from pathlib import Path -from unittest.mock import MagicMock, Mock, patch +from unittest.mock import Mock, patchcortex/tutor/tests/test_integration.py (2)
10-10: Remove unused import.
MagicMockis imported but not used.Suggested fix
-from unittest.mock import MagicMock, Mock, patch +from unittest.mock import Mock, patch
205-222: Consider removing duplicate branding tests.These tests duplicate coverage from
tests/tutor/test_branding.py. Consider removing them to reduce maintenance overhead, or keep them intentionally as integration-level smoke tests.tests/tutor/test_interactive_tutor.py (2)
1-11: Unused import detected.The
callimport fromunittest.mockis not used anywhere in this file.🧹 Remove unused import
-from unittest.mock import MagicMock, Mock, call, patch +from unittest.mock import MagicMock, Mock, patch
247-264: Test coverage for None lesson is incomplete.This test only exercises 4 of the 6 helper methods (
_show_concepts,_show_examples,_run_tutorial,_show_best_practices). Consider adding_ask_questionand_show_progressfor completeness.Additionally, the test only verifies no exceptions are raised but doesn't assert expected behavior (e.g., that appropriate "no data" messages are printed).
🧪 Extended test with assertions
def test_methods_with_no_lesson(self, mock_agent_class): """Test methods handle None lesson gracefully.""" from cortex.tutor.agents.tutor_agent import InteractiveTutor tutor = InteractiveTutor("docker") tutor.lesson = None tutor.agent = Mock() + tutor.agent.get_progress.return_value = {"success": False} # These should not raise errors - tutor._show_concepts() - tutor._show_examples() - tutor._run_tutorial() - tutor._show_best_practices() + with patch("cortex.tutor.agents.tutor_agent.tutor_agent.tutor_print") as mock_print: + tutor._show_concepts() + tutor._show_examples() + tutor._run_tutorial() + tutor._show_best_practices() + tutor._show_progress()tests/tutor/test_agent_methods.py (1)
411-441: State helpers well-tested, buthas_critical_erroris missing a direct test.The
has_critical_errorfunction is imported (line 33) but not directly tested inTestStateHelpers. It's indirectly tested viaTestRouting.test_route_after_plan_fail_on_error, but a direct unit test would improve coverage.🧪 Add direct test for has_critical_error
def test_has_critical_error_true(self): """Test has_critical_error returns True for non-recoverable errors.""" state = create_initial_state("docker") add_error(state, "test", "Critical error", recoverable=False) assert has_critical_error(state) is True def test_has_critical_error_false(self): """Test has_critical_error returns False for recoverable errors.""" state = create_initial_state("docker") add_error(state, "test", "Minor error", recoverable=True) assert has_critical_error(state) is Falsetests/tutor/test_integration.py (2)
205-222: Branding tests verify no errors but lack output assertions.The
capsys.readouterr()result is captured but not verified. While Rich console output is complex, consider at minimum asserting the output is non-empty for success cases, or use Rich'sConsolewith aStringIOfor testable output.🧪 Add minimal assertions
def test_tutor_print_success(self, capsys): """Test tutor_print with success status.""" tutor_print("Test message", "success") captured = capsys.readouterr() - # Rich console output is complex, just ensure no errors + # Rich console output is complex, verify something was printed + assert len(captured.out) > 0 or len(captured.err) > 0
298-365: Good end-to-end test for cache-hit workflow.The test properly exercises the Plan→Load→Reflect flow with mocked tools and verifies state transitions. The note about removing real API tests is appropriate for CI.
Consider adding a similar test for the cache-miss path (Plan→Generate→Reflect) to cover both main workflows.
cortex/tutor/tests/test_progress_tracker.py (1)
290-351: Redundant imports inside test methods.
Mockandpatchare imported inside each test method. Since these are already available in the module scope (line 9), the local imports are unnecessary.🧹 Remove redundant imports
def test_get_learning_progress(self, temp_db): """Test get_learning_progress function.""" - from unittest.mock import Mock, patch - # Mock the global config to use temp_db mock_config = Mock()Apply similar changes to
test_mark_topic_completedandtest_get_package_stats.cortex/tutor/tests/test_tools.py (1)
277-310: ConversationHandler tests use__new__to bypass initialization.While using
__new__to create uninitialized instances works for testing, it's fragile and couples tests to implementation details. Consider adding a factory method or making the handler more testable.# Alternative: Initialize properly with mocked config with patch("cortex.tutor.tools.agentic.qa_handler.get_config") as mock_config: mock_config.return_value = Mock(anthropic_api_key="test", model="test") with patch("cortex.tutor.tools.agentic.qa_handler.ChatAnthropic"): handler = ConversationHandler() handler.history = [] # Reset history for testcortex/tutor/config.py (2)
45-49: Direct mutation inmodel_post_initworks but considermodel_validator.Pydantic v2 allows mutation in
model_post_init, but using amodel_validatorwithmode='after'is more idiomatic for computed defaults.♻️ Alternative using model_validator
from pydantic import model_validator @model_validator(mode='after') def set_db_path_default(self) -> 'Config': """Set default db_path if not provided.""" if self.db_path is None: object.__setattr__(self, 'db_path', self.data_dir / "tutor_progress.db") return self
100-108: Return type inconsistency with field type.
get_db_path()returnsPathbutdb_pathfield is typed asPath | None. Whilemodel_post_initensures it's always set, the type checker may flag this. Consider adding an assertion or adjusting the type.🔧 Add runtime assertion for type safety
def get_db_path(self) -> Path: """ Get the full path to the SQLite database. Returns: Path: Full path to tutor_progress.db """ self.ensure_data_dir() + assert self.db_path is not None, "db_path should be set by model_post_init" return self.db_pathcortex/tutor/branding.py (2)
7-7: Remove unusedOptionalimport.The code uses modern union syntax (
str | None) throughout, soOptionalis not needed.Proposed fix
-from typing import Literal, Optional +from typing import Literal
218-234: Unuseddescriptionparameter.The
descriptionparameter is accepted but not used. If the intent is to set a default task description, it should be returned with a pre-added task or documented that callers should useprogress.add_task(description).Option 1: Remove unused parameter
-def create_progress_bar(description: str = "Processing") -> Progress: +def create_progress_bar() -> Progress: """ Create a Rich progress bar for long-running operations. - Args: - description: Description text for the progress bar. - Returns: Progress: Configured Rich Progress instance. """Option 2: Use the parameter
+from typing import Tuple + -def create_progress_bar(description: str = "Processing") -> Progress: +def create_progress_bar(description: str = "Processing") -> Tuple[Progress, int]: """ Create a Rich progress bar for long-running operations. Args: description: Description text for the progress bar. Returns: - Progress: Configured Rich Progress instance. + Tuple of (Progress instance, task_id). """ - return Progress( + progress = Progress( SpinnerColumn(), TextColumn("[progress.description]{task.description}"), BarColumn(), TaskProgressColumn(), console=console, ) + task_id = progress.add_task(description, total=100) + return progress, task_idtests/tutor/test_progress_tracker.py (1)
7-7: Remove unusedosimport.The
osmodule is imported but not used in this file.Proposed fix
-import os import tempfilecortex/tutor/agents/tutor_agent/tutor_agent.py (1)
86-92: Calling private_runmethod directly on LangChain tools.The
_runmethod is a private API of LangChain'sBaseTool. For tool invocation, the public API isrun()orinvoke(). Using private methods may break if the LangChain library changes its internal implementation.♻️ Suggested refactor
- self.progress_tool._run( - "update_progress", - package_name=package_name, - topic="overview", - ) + self.progress_tool.run( + "update_progress", + package_name=package_name, + topic="overview", + )Apply the same pattern to all other
._run()calls in this file (Lines 155, 156, 165, 177, 192-197, 210, 220).cortex/tutor/tools/deterministic/lesson_loader.py (2)
100-106: Async method delegates to synchronous implementation.The
_arunmethod simply calls the sync_runmethod, which could block the event loop if the SQLite operations are slow. For a deterministic cache lookup, this is likely acceptable, but consider documenting this limitation.
125-129: Silent exception swallowing incache_lesson.Catching all exceptions and returning
Falsehides the root cause of failures. Consider logging the exception for debugging purposes.♻️ Suggested improvement
+import logging + +logger = logging.getLogger(__name__) + def cache_lesson( self, package_name: str, lesson: dict[str, Any], ttl_hours: int = 24, ) -> bool: ... try: self.store.cache_lesson(package_name, lesson, ttl_hours) return True - except Exception: + except Exception as e: + logger.warning("Failed to cache lesson for %s: %s", package_name, e) return Falsecortex/tutor/tools/agentic/lesson_generator.py (2)
21-36: Unused function_load_prompt_template.This function is defined but never called. The prompts are instead hardcoded in
_get_system_promptand_get_generation_promptmethods. Either use this function or remove it.♻️ Option 1: Remove unused code
-# Load prompt template -def _load_prompt_template() -> str: - """Load the lesson generator prompt from file.""" - prompt_path = Path(__file__).parent.parent.parent / "prompts" / "tools" / "lesson_generator.md" - if prompt_path.exists(): - return prompt_path.read_text() - # Fallback inline prompt - return """You are a lesson content generator. Generate comprehensive educational content - for the package: {package_name} - - Student level: {student_level} - Learning style: {learning_style} - Focus areas: {focus_areas} - - Return a JSON object with: summary, explanation, use_cases, best_practices, - code_examples, tutorial_steps, installation_command, confidence.""" -
82-140: DRY violation:_runand_arunhave duplicated logic.The async
_arunmethod duplicates most of the logic from_run, including prompt construction, chain setup, and error handling. Consider extracting shared logic.♻️ Suggested refactor
def _build_chain(self): """Build the LLM chain for lesson generation.""" prompt = ChatPromptTemplate.from_messages( [ ("system", self._get_system_prompt()), ("human", self._get_generation_prompt()), ] ) return prompt | self.llm | JsonOutputParser() def _build_invoke_params(self, package_name, student_level, learning_style, focus_areas, skip_areas): """Build parameters for chain invocation.""" return { "package_name": package_name, "student_level": student_level, "learning_style": learning_style, "focus_areas": ", ".join(focus_areas or []) or "all topics", "skip_areas": ", ".join(skip_areas or []) or "none", } def _run(self, package_name, student_level="beginner", ...): try: chain = self._build_chain() params = self._build_invoke_params(...) result = chain.invoke(params) # ...Also applies to: 142-184
tests/tutor/test_tools.py (1)
280-309: Fragile test setup using__new__to bypass initialization.Using
__new__to create objects without calling__init__is fragile and can break if the class implementation changes. Consider using proper mocking or fixtures.♻️ Suggested improvement
class TestConversationHandler: """Tests for ConversationHandler.""" def test_build_context_empty(self): """Test context building with empty history.""" - with patch("cortex.tutor.tools.agentic.qa_handler.get_config"): - handler = ConversationHandler.__new__(ConversationHandler) - handler.history = [] - + with patch("cortex.tutor.tools.agentic.qa_handler.get_config") as mock_config: + mock_config.return_value = Mock( + anthropic_api_key="test_key", + model="claude-sonnet-4-20250514", + ) + with patch("cortex.tutor.tools.agentic.qa_handler.ChatAnthropic"): + handler = ConversationHandler(package_name="test") + handler.history = [] + context = handler._build_context() assert "Starting fresh" in contextcortex/tutor/tools/agentic/examples_provider.py (2)
7-8: Unused import:Path.The
Pathimport frompathlibis not used anywhere in this file.♻️ Suggested fix
-from pathlib import Path from typing import Any
59-113: DRY violation:_runand_arunduplicate logic.Same issue as in
lesson_generator.py. The async method duplicates the sync implementation. Consider extracting shared chain-building logic.Also applies to: 115-157
cortex/tutor/agents/tutor_agent/graph.py (2)
374-388: Singleton graph creation is not thread-safe.The
get_tutor_graph()function uses a global variable without synchronization. In a multi-threaded environment, multiple threads could create the graph simultaneously.♻️ Suggested thread-safe implementation
+import threading + # Create singleton graph instance _graph = None +_graph_lock = threading.Lock() def get_tutor_graph() -> StateGraph: """ Get the singleton Tutor Agent graph. Returns: Compiled StateGraph. """ global _graph - if _graph is None: - _graph = create_tutor_graph() + if _graph is None: + with _graph_lock: + if _graph is None: # Double-check locking + _graph = create_tutor_graph() return _graph
44-45: Using private_runmethod on LangChain tools throughout graph nodes.Multiple nodes call
tool._run()which is a private API. Consider using the publicrun()orinvoke()methods for better compatibility with future LangChain versions.Also applies to: 62-63, 122-123, 173-174
cortex/tutor/cli.py (3)
155-210: Consider handling missingvalidation_passedkey more defensively.If
agent.ask()returns a malformed response without thevalidation_passedkey,.get("validation_passed")returnsNone, which is falsy and falls through to the error case. This is reasonable behavior, but you may want to log or handle unexpected response formats explicitly for debugging purposes.
213-240: Docstring is missing theverboseparameter.The
cmd_list_packagesfunction accepts averboseparameter but the docstring doesn't document it.📝 Suggested fix
def cmd_list_packages(verbose: bool = False) -> int: """ List packages that have been studied. + Args: + verbose: Enable verbose output. + Returns: Exit code. """
243-313: Docstring is missing theverboseparameter documentation.Similar to
cmd_list_packages, theverboseparameter is not documented in the docstring.📝 Suggested fix
def cmd_progress(package: str | None = None, verbose: bool = False) -> int: """ Show learning progress. Args: package: Optional package filter. + verbose: Enable verbose output. Returns: Exit code. """cortex/tutor/tools/deterministic/progress_tracker.py (2)
115-117: Async implementation blocks the event loop.The
_arunmethod calls the synchronous_runmethod directly, which will block the event loop if_runperforms I/O (SQLite operations). For a truly async implementation, consider usingasyncio.to_thread()or an async-compatible SQLite library.♻️ Suggested fix to avoid blocking the event loop
async def _arun(self, *args: Any, **kwargs: Any) -> dict[str, Any]: """Async version - delegates to sync implementation.""" - return self._run(*args, **kwargs) + import asyncio + return await asyncio.to_thread(self._run, *args, **kwargs)
301-346: Convenience functions create new tool instances per call.Each convenience function (
get_learning_progress,mark_topic_completed,get_package_stats) instantiates a newProgressTrackerTooland thus a newSQLiteStore. This is inefficient for repeated calls and may have connection overhead. Consider caching or accepting an optional tool/store parameter.cortex/tutor/tools/agentic/qa_handler.py (3)
1-8: Unused import:Pathis imported but never used.The
Pathimport frompathlibis not used in this file.🧹 Remove unused import
-from pathlib import Path from typing import Any
118-162: Code duplication between_runand_arun.The sync and async methods have nearly identical logic. Consider extracting the common prompt/chain setup into a helper method.
♻️ Suggested refactor to reduce duplication
def _build_chain(self): """Build the prompt chain for Q&A.""" prompt = ChatPromptTemplate.from_messages( [ ("system", self._get_system_prompt()), ("human", self._get_qa_prompt()), ] ) return prompt | self.llm | JsonOutputParser() def _build_invoke_params(self, package_name, question, learning_style, mastered_concepts, weak_concepts, lesson_context): """Build parameters for chain invocation.""" return { "package_name": package_name, "question": question, "learning_style": learning_style, "mastered_concepts": ", ".join(mastered_concepts or []) or "none specified", "weak_concepts": ", ".join(weak_concepts or []) or "none specified", "lesson_context": lesson_context or "starting fresh", }
268-344: Unbounded history growth in ConversationHandler.The
historylist grows unboundedly. While_build_contextonly uses the last 3 exchanges, the full history is retained. Consider limiting history size to prevent memory growth in long sessions.♻️ Suggested fix to limit history size
+ MAX_HISTORY_SIZE = 10 # Keep last 10 exchanges + def ask( self, question: str, # ... params ) -> dict[str, Any]: # ... existing code ... # Update history if result.get("success"): self.history.append( { "question": question, "answer": result["answer"].get("answer", ""), } ) + # Trim history to prevent unbounded growth + if len(self.history) > self.MAX_HISTORY_SIZE: + self.history = self.history[-self.MAX_HISTORY_SIZE:] return resulttests/tutor/test_branding.py (3)
7-8: Unused imports:StringIO,MagicMock, andMockare imported but never used.These imports are not utilized in the test file.
🧹 Remove unused imports
-from io import StringIO -from unittest.mock import MagicMock, Mock, patch +from unittest.mock import patch
44-70: Tests lack assertions to verify output content.The
TestTutorPrinttests call the functions but don't assert anything about the output. While smoke testing is valuable, consider adding at least basic assertions usingcapsys.readouterr()to verify expected output patterns.💡 Example of adding assertions
def test_tutor_print_success(self, capsys): """Test success status print.""" tutor_print("Test message", "success") captured = capsys.readouterr() assert "Test message" in captured.out or len(captured.out) > 0
73-91: Tests lack meaningful assertions.Similar to
TestTutorPrint, these tests only verify that functions don't raise exceptions. Consider adding assertions to verify output contains expected content.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (46)
cortex/cli.pycortex/tutor/__init__.pycortex/tutor/agents/tutor_agent/__init__.pycortex/tutor/agents/tutor_agent/graph.pycortex/tutor/agents/tutor_agent/state.pycortex/tutor/agents/tutor_agent/tutor_agent.pycortex/tutor/branding.pycortex/tutor/cli.pycortex/tutor/config.pycortex/tutor/contracts/lesson_context.pycortex/tutor/contracts/progress_context.pycortex/tutor/memory/sqlite_store.pycortex/tutor/tests/test_agent_methods.pycortex/tutor/tests/test_agentic_tools.pycortex/tutor/tests/test_branding.pycortex/tutor/tests/test_cli.pycortex/tutor/tests/test_deterministic_tools.pycortex/tutor/tests/test_integration.pycortex/tutor/tests/test_interactive_tutor.pycortex/tutor/tests/test_progress_tracker.pycortex/tutor/tests/test_tools.pycortex/tutor/tests/test_tutor_agent.pycortex/tutor/tests/test_validators.pycortex/tutor/tools/__init__.pycortex/tutor/tools/agentic/__init__.pycortex/tutor/tools/agentic/examples_provider.pycortex/tutor/tools/agentic/lesson_generator.pycortex/tutor/tools/agentic/qa_handler.pycortex/tutor/tools/deterministic/__init__.pycortex/tutor/tools/deterministic/lesson_loader.pycortex/tutor/tools/deterministic/progress_tracker.pycortex/tutor/tools/deterministic/validators.pydocs/AI_TUTOR.mdrequirements.txttests/tutor/__init__.pytests/tutor/test_agent_methods.pytests/tutor/test_agentic_tools.pytests/tutor/test_branding.pytests/tutor/test_cli.pytests/tutor/test_deterministic_tools.pytests/tutor/test_integration.pytests/tutor/test_interactive_tutor.pytests/tutor/test_progress_tracker.pytests/tutor/test_tools.pytests/tutor/test_tutor_agent.pytests/tutor/test_validators.py
✅ Files skipped from review due to trivial changes (1)
- tests/tutor/init.py
🚧 Files skipped from review as they are similar to previous changes (9)
- docs/AI_TUTOR.md
- cortex/tutor/tests/test_cli.py
- cortex/tutor/tests/test_agentic_tools.py
- cortex/tutor/tests/test_tutor_agent.py
- requirements.txt
- cortex/tutor/init.py
- cortex/tutor/agents/tutor_agent/init.py
- cortex/tutor/tests/test_interactive_tutor.py
- cortex/cli.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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/tutor/test_agentic_tools.pycortex/tutor/tests/test_integration.pytests/tutor/test_tutor_agent.pycortex/tutor/tools/agentic/__init__.pytests/tutor/test_cli.pycortex/tutor/tests/test_agent_methods.pytests/tutor/test_interactive_tutor.pycortex/tutor/tests/test_progress_tracker.pycortex/tutor/tools/deterministic/__init__.pycortex/tutor/config.pytests/tutor/test_deterministic_tools.pycortex/tutor/tools/__init__.pytests/tutor/test_tools.pycortex/tutor/contracts/progress_context.pycortex/tutor/tests/test_tools.pycortex/tutor/branding.pytests/tutor/test_integration.pytests/tutor/test_progress_tracker.pytests/tutor/test_agent_methods.pycortex/tutor/tests/test_validators.pycortex/tutor/agents/tutor_agent/tutor_agent.pycortex/tutor/memory/sqlite_store.pycortex/tutor/agents/tutor_agent/state.pycortex/tutor/tools/deterministic/progress_tracker.pycortex/tutor/agents/tutor_agent/graph.pycortex/tutor/contracts/lesson_context.pycortex/tutor/tools/deterministic/validators.pycortex/tutor/tools/agentic/lesson_generator.pycortex/tutor/tools/deterministic/lesson_loader.pycortex/tutor/tools/agentic/examples_provider.pycortex/tutor/tests/test_deterministic_tools.pycortex/tutor/cli.pytests/tutor/test_validators.pycortex/tutor/tools/agentic/qa_handler.pytests/tutor/test_branding.pycortex/tutor/tests/test_branding.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/tutor/test_agentic_tools.pytests/tutor/test_tutor_agent.pytests/tutor/test_cli.pytests/tutor/test_interactive_tutor.pytests/tutor/test_deterministic_tools.pytests/tutor/test_tools.pytests/tutor/test_integration.pytests/tutor/test_progress_tracker.pytests/tutor/test_agent_methods.pytests/tutor/test_validators.pytests/tutor/test_branding.py
{setup.py,setup.cfg,pyproject.toml,**/__init__.py}
📄 CodeRabbit inference engine (AGENTS.md)
Use Python 3.10 or higher as the minimum supported version
Files:
cortex/tutor/tools/agentic/__init__.pycortex/tutor/tools/deterministic/__init__.pycortex/tutor/tools/__init__.py
🧠 Learnings (1)
📚 Learning: 2025-12-11T12:03:24.071Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T12:03:24.071Z
Learning: Applies to tests/**/*.py : Maintain >80% test coverage for pull requests
Applied to files:
cortex/tutor/tests/test_progress_tracker.py
🔇 Additional comments (77)
cortex/tutor/tests/test_validators.py (2)
26-90: Thorough package name validation tests.Good coverage of edge cases including valid names, empty input, whitespace, length limits, blocked patterns, and invalid characters. The blocked pattern tests appropriately cover dangerous shell commands.
266-302: Well-structured integration tests for validation workflow.The
TestGetValidationErrorsandTestValidateAllclasses properly test the aggregation and combined validation flows. Good use of multiple validation types in combination.cortex/tutor/memory/sqlite_store.py (2)
362-390: Race condition handling in_create_default_profileis well implemented.Good use of
INSERT OR IGNOREfollowed by a re-fetch to handle concurrent profile creation attempts. The re-entrant lock (RLock) also helps with nested calls.
474-496: Lesson cache expiration logic is correct.The TTL-based cache properly uses UTC timestamps and the expiration check in
get_cached_lessoncorrectly filters expired entries.cortex/tutor/tools/agentic/__init__.py (1)
1-17: Clean package initialization with proper exports.Well-structured
__init__.pywith a clear docstring explaining the purpose of agentic tools. The__all__properly defines the public API surface.cortex/tutor/tests/test_deterministic_tools.py (3)
23-58: Good cache behavior tests for LessonLoaderTool.Tests properly verify cache miss, cache storage/retrieval, and
force_freshbypass. The use of a temporary database fixture ensures test isolation.
174-179: Good coverage of error handling for invalid actions.The test properly verifies that invalid actions return a failure response with an error message.
89-95: Ensure fallback validation is exhaustive.The test iterates over
FALLBACK_LESSONSbut only checks forpackage_nameandsummaryfields. Consider verifying all required fields that downstream code depends on.tests/tutor/test_validators.py (1)
1-303: Verify whether duplicate test files exist.This review raises a concern about potential duplicate test files. Before acting on this claim:
- Confirm that both
cortex/tutor/tests/test_validators.pyandtests/tutor/test_validators.pyexist in the current state- If both exist, compare them to verify they are identical
- If confirmed as duplicates, determine which should be removed based on your refactoring intent
tests/tutor/test_agentic_tools.py (1)
12-197: Test structure and coverage look solid.The tests properly mock external dependencies (
get_config,ChatAnthropic) and validate_structure_responsebehavior across full and minimal response scenarios for each agentic tool. TheConversationHandlercontext-building tests cover both empty and populated history states.cortex/tutor/tools/deterministic/__init__.py (1)
1-17: Clean package initialization with appropriate exports.The module docstring clearly explains that these are deterministic (non-LLM) tools, and
__all__properly exposes the public API.cortex/tutor/tests/test_agent_methods.py (1)
37-441: Comprehensive test coverage for TutorAgent orchestration.Tests thoroughly cover agent methods, graph node behaviors (success/failure/exception paths), routing decisions, and state helpers. The mocking strategy properly isolates units from external dependencies.
cortex/tutor/tests/test_branding.py (1)
30-227: UI smoke tests provide reasonable coverage.The tests verify that branding functions execute without errors across various input scenarios. For Rich console output, smoke testing is a practical approach since exact output assertions would be brittle.
tests/tutor/test_tutor_agent.py (1)
33-321: Well-structured tests covering state management, graph nodes, and routing.Tests comprehensively validate state creation, error/checkpoint/cost accumulation, critical error detection, and routing logic. The integration tests properly verify input validation.
tests/tutor/test_deterministic_tools.py (1)
1-179: Excellent test coverage for deterministic tools.Tests properly isolate database operations using temporary directories, cover all
ProgressTrackerToolactions including error handling for invalid actions, and verify fallback lesson content including case-insensitivity and required fields. The fixture pattern ensures clean test isolation.cortex/tutor/tests/test_integration.py (1)
298-364: Solid end-to-end workflow test.The test validates the complete cache-hit lesson workflow through
plan_node → load_cache_node → reflect_node, verifying state transitions and final output validation. Good use of mocking to isolate from actual LLM calls.tests/tutor/test_interactive_tutor.py (1)
84-123: Well-structured test fixture.The
mock_tutorfixture provides comprehensive mock data covering all lesson components (examples, tutorial steps, best practices) with properly configured agent mocks. This enables clean, focused test methods.tests/tutor/test_agent_methods.py (2)
37-198: Good coverage of TutorAgent public API.The tests comprehensively cover all public methods (
teach,ask,get_profile,update_learning_style,mark_completed,reset_progress,get_packages_studied) with proper mocking of dependencies.
200-259: Comprehensive node testing with error scenarios.The
generate_lesson_nodetests cover success, API failure, and exception handling paths. The caching behavior on success is properly verified.tests/tutor/test_integration.py (1)
32-81: Config tests are comprehensive.Good coverage of environment loading, API key validation, path expansion, and directory creation with proper use of
monkeypatchandtempfilefor isolation.tests/tutor/test_cli.py (3)
24-87: Parser tests are thorough.All CLI flags and arguments are tested including edge cases like
--resetwith and without package argument.
89-149: Good security test for blocked patterns.The
test_blocked_package_nametest verifies that potentially dangerous input like"rm -rf /"is rejected, which is important for security.
401-453: Main entry point routing is well-tested.Tests verify that CLI arguments are correctly routed to their respective command handlers.
cortex/tutor/tests/test_progress_tracker.py (2)
47-212: Excellent SQLiteStore test coverage.Comprehensive testing of all store operations including CRUD, statistics, profile management, lesson caching with TTL, and reset functionality.
214-288: Good action-based testing of ProgressTrackerTool.All supported actions are tested, including error cases for unknown actions and missing required parameters.
cortex/tutor/tests/test_tools.py (1)
31-122: Good caching and fallback behavior testing.Tests cover cache hit/miss scenarios, force-refresh, fallback templates, and the priority order (cache → fallback → none).
cortex/tutor/config.py (2)
127-141: DocstringRaisesis accurate but indirect.The docstring correctly notes
ValueErrorcan be raised, which happens viaConfig.from_env(). This is accurate.
1-16: Well-structured configuration module.Good use of Pydantic for validation, proper docstrings, type hints, and secure defaults (0o700 permissions for data directory). The environment-based configuration with optional API key requirement enables the lazy import pattern mentioned in the PR objectives.
cortex/tutor/tools/__init__.py (1)
1-20: LGTM!Clean package initialization with proper docstring and centralized exports. The
__all__list correctly exposes the public API for both deterministic and agentic tools.cortex/tutor/contracts/lesson_context.py (2)
101-131: Well-designed serialization and utility methods.The
to_json(),from_json(), andto_display_dict()methods provide a clean API for caching and presentation. Good use of Pydantic's built-in serialization.
56-75: Pydantic v2max_lengthon list fields correctly constrains list size.In Pydantic v2,
Field(max_length=N)on a list field constrains the number of items in the list, not the length of individual string elements. The code is using this correctly and requires no changes.Likely an incorrect or invalid review comment.
cortex/tutor/branding.py (2)
34-50: Well-designed status printing utility.Clean implementation with proper fallback to default config. The status-emoji mapping provides a consistent UI experience.
193-216: Good defensive error handling inget_user_input.Properly handles
EOFErrorandKeyboardInterruptto gracefully cancel operations without crashing.tests/tutor/test_progress_tracker.py (3)
27-44: Well-structured test fixtures.Good use of
tempfile.TemporaryDirectorycontext manager withyieldfor automatic cleanup. The fixture chain (temp_db→store/tracker) provides proper test isolation.
278-288: Good error case coverage.Tests for unknown actions and missing required parameters ensure the tool gracefully handles invalid inputs.
293-350: Proper mocking pattern for convenience functions.Good use of
patchto substitute config, ensuring tests are isolated from the actual database path. The tests verify end-to-end behavior through the public API.cortex/tutor/contracts/progress_context.py (2)
31-63: Well-designed computed fields with proper edge case handling.The
@computed_fielddecorators correctly implement derived properties. Theis_complete()method properly returnsFalsefor empty topic lists, preventing false positives.
159-176: Clean factory method pattern.
from_resultsproperly guards against division by zero and provides sensible defaults. The 70% pass threshold is a reasonable default for quiz assessment.cortex/tutor/agents/tutor_agent/tutor_agent.py (3)
265-275: Lazy imports insidestart()method are appropriate.The PR objectives mention "lazy imports to allow listing/progress commands without an API key." This approach is intentional and correctly defers heavy imports until interactive mode is needed.
309-313: Consider validating menu option range before conversion.The input handling catches
ValueErrorfor non-numeric input, but the range check happens after successful conversion. This is acceptable but could provide clearer feedback.
23-48: TutorAgent class is well-structured with proper docstrings and type hints.The class follows PEP 8 style, includes comprehensive docstrings with examples, and uses type hints throughout. The initialization pattern is clean.
cortex/tutor/tools/deterministic/lesson_loader.py (2)
163-232: FALLBACK_LESSONS provides good offline resilience.The predefined fallback lessons for common packages (docker, git, nginx) ensure the tutor can function without API access. The lower confidence score (0.7) appropriately signals these are fallback content.
18-49: LessonLoaderTool class is well-documented with proper type hints.The class follows PEP 8, has comprehensive docstrings, and uses Pydantic fields appropriately. The Config class enables arbitrary types for the SQLiteStore field.
cortex/tutor/tools/agentic/lesson_generator.py (2)
262-276: Good defensive coding in_structure_response.The method properly handles missing fields with defaults, clamps confidence to valid range (0.0-1.0), and limits list sizes to prevent unbounded responses. This is well-implemented.
186-202: System prompt has appropriate safety rules.The critical rules against fabricating features, URLs, or versions are important for educational content integrity. Good security posture.
tests/tutor/test_tools.py (2)
24-28: Good use of pytest fixture for test isolation.The
temp_dbfixture properly creates an isolated temporary database for each test, ensuring tests don't interfere with each other.
127-181: Test mocking is thorough and at the correct level.The tests mock
ChatAnthropicandget_configat the appropriate module level, and verify the tool's response structure without requiring actual LLM calls.cortex/tutor/tools/agentic/examples_provider.py (2)
159-174: System prompt enforces good safety practices.The critical rules against inventing flags, using real credentials, and flagging dangerous commands are appropriate for educational content generation.
208-236: Good defensive coding in_structure_response.The method properly validates example structure, limits list sizes, and provides sensible defaults. The confidence clamping is correctly implemented.
cortex/tutor/agents/tutor_agent/graph.py (2)
200-249: Reflect node has well-designed validation logic.The confidence calculation accounting for errors and cache staleness, combined with comprehensive output preparation, demonstrates good design. The validation_passed flag provides clear success/failure indication.
315-371: Graph structure is well-organized with clear entry point and edges.The workflow correctly implements the Plan→Act→Reflect pattern with proper conditional routing and terminal states. The conditional edges provide flexibility for different session types.
cortex/tutor/agents/tutor_agent/state.py (4)
50-105: Well-designed TypedDict state schema.The
TutorAgentStateprovides a clear contract for the workflow state with comprehensive documentation. Usingtotal=Falseallows optional fields while maintaining type safety.
107-150: Factory function provides clean state initialization.
create_initial_statesets sensible defaults for all fields, ensuring the state is always in a valid initial configuration. The explicit initialization prevents undefined state issues.
153-171: Helper functions mutate state in-place.The
add_error,add_checkpoint, andadd_costfunctions modify the state dictionary directly. This is appropriate for LangGraph workflows where state is passed through nodes and accumulated.
42-47: ErrorState usestotal=True(default) enforcing required fields.Unlike other state TypedDicts,
ErrorStaterequires all fields (node,error,recoverable), which is appropriate since an error entry should always have complete information.cortex/tutor/cli.py (5)
1-33: LGTM! Well-structured module docstring and imports.The module docstring clearly documents CLI usage patterns. Lazy imports strategy for API-key-dependent modules is a good design choice. The
DEFAULT_TUTOR_TOPICSconstant is appropriately defined at module level.
35-109: LGTM! Argument parser is well-configured.The parser correctly handles all required CLI options including version, verbose mode, package name, list, progress, reset, fresh, and question flags. The
--resetwithnargs="?"andconst="__all__"is a nice pattern for optional argument handling.
112-152: LGTM! Solid error handling in teach command.The function properly validates input before expensive imports, handles
ValueError,KeyboardInterrupt, and general exceptions with appropriate exit codes. The lazy import pattern ensures API key is only required when actually starting a session.
316-347: LGTM! Reset command with proper confirmation flow.The confirmation prompt before destructive operations is good UX practice. The scope calculation handles both
Noneand"__all__"correctly.
350-392: LGTM! Clean command dispatch logic.The
mainfunction correctly handles argument parsing, conditional banner display, and command routing. The fallback toparser.print_help()when no command is specified is appropriate.cortex/tutor/tools/deterministic/validators.py (7)
15-34: LGTM! Comprehensive security patterns for input validation.The blocked patterns list covers common injection vectors (shell commands, fork bombs, Python code injection). This is a solid defense-in-depth approach for a CLI tool that might pass user input to an LLM.
36-48: LGTM! Well-defined validation patterns and categories.The regex pattern for valid package names is appropriately restrictive while allowing common naming conventions (dots, hyphens, underscores). The package categories list provides good context for documentation.
51-93: LGTM! Thorough package name validation with clear error messages.The function correctly handles empty input, length limits, blocked patterns, and format validation. The docstring includes helpful examples demonstrating expected behavior.
96-135: LGTM! Generic input validation with configurable parameters.The function properly handles the
allow_emptyflag and applies length limits before checking blocked patterns.
138-210: LGTM! Specialized validators with appropriate constraints.The
validate_question,validate_topic,validate_score, andvalidate_learning_stylefunctions properly delegate to base validators or implement specific rules. The score validation correctly handles both int and float types.
213-277: LGTM! Input sanitization and package name extraction utilities.The
sanitize_inputfunction removes null bytes and enforces length limits. Theextract_package_namefunction uses a reasonable set of patterns to extract package names from natural language input.
280-367: LGTM! Clean aggregation utilities with ValidationResult class.The
get_validation_errorsandvalidate_allfunctions provide convenient batch validation. TheValidationResultclass implements__bool__and__str__for ergonomic usage.cortex/tutor/tools/deterministic/progress_tracker.py (5)
23-59: LGTM! Well-documented tool class with proper initialization.The class docstring clearly explains the tool's purpose and constraints (no LLM calls). The initialization correctly handles optional
db_pathwith fallback to config default.
61-113: LGTM! Clean action dispatcher with error handling.The dispatcher pattern is well-implemented with a dictionary mapping actions to handlers. Unknown actions return helpful error messages with valid action lists.
119-164: LGTM! Progress retrieval handlers with proper validation.Both
_get_progressand_get_all_progresscorrectly handle missing required parameters and return structured responses.
166-213: LGTM! Progress update handlers with proper data preservation.The
_mark_completedand_update_progresshandlers correctly preserve existing values when updating progress. The time accumulation logic in_update_progressis correctly implemented.
215-295: LGTM! Stats and profile management handlers.The handlers correctly implement statistics retrieval, profile management, and progress reset functionality. Error handling is consistent across all methods.
cortex/tutor/tools/agentic/qa_handler.py (4)
59-116: LGTM! Robust error handling and response structuring.The
_runmethod properly wraps LLM calls in try-except, structures responses consistently, and includes cost estimation. The chain composition withprompt | self.llm | JsonOutputParser()is idiomatic LangChain.
164-213: LGTM! Well-designed system and Q&A prompts.The system prompt includes critical safety rules (no fabrication, no fake URLs, confidence expression). The Q&A prompt template defines a clear JSON schema for structured responses.
215-241: LGTM! Response structuring with validation.The
_structure_responsemethod properly clamps confidence to [0.0, 1.0], limits array lengths for related topics and suggestions, and safely handles missing or malformed code examples.
36-57: Model name is valid and pricing estimate is reasonable.The model identifier
claude-sonnet-4-20250514is confirmed as a valid current Anthropic model. Current pricing is $3.00 per 1M input tokens and $15.00 per 1M output tokens. With max_tokens set to 2048, the estimated cost of ~$0.02 per question is reasonable for typical usage patterns.tests/tutor/test_branding.py (1)
229-249: LGTM! User input tests with proper mocking and assertions.These tests correctly mock
builtins.inputand verify the expected behavior. The test at line 245-249 correctly tests that raw input is returned (checking"spaced" in resultrather than exact match).
|
@Anshgrover23 All checks are now passing! ✅ Summary of updates:
Ready for review. Please take a look when you get a chance. Thanks! |
Anshgrover23
left a comment
Anshgrover23
left a comment
There was a problem hiding this comment.
@srikrishnavansi Your SonarQubeCloud Quality Gate is failing kindly address that issues. Also address coderabbit comments that are still open.
|
Summary of Changes
@Anshgrover23 Please let me know if any further changes are needed! |
mikejmorgan-ai
left a comment
mikejmorgan-ai
left a comment
There was a problem hiding this comment.
Review: APPROVED ✅
AI-Powered Installation Tutor (PR #566)
Code Quality
- ✅ Clean architecture: agent.py, cli.py, config.py, validators.py, sqlite_store.py, tools.py
- ✅ Strong input validation with blocked dangerous patterns (rm -rf, fork bombs, eval injection)
- ✅ Secure file permissions (0o700 for data directory)
- ✅ Proper error handling with user-friendly messages
- ✅ Lazy imports for LLM components (only loaded when API key available)
Security
- ✅ BLOCKED_PATTERNS prevents command injection attempts
- ✅ Package name validation: alphanumeric + dots/hyphens/underscores only
- ✅ Input length limits prevent abuse
- ✅ Sanitization removes null bytes and truncates long inputs
- ✅ API key validated before use
Test Coverage
- ✅ 167 tests all passing
- ✅ Tests cover: CLI, tools, validators, progress tracking, LLM integration
- ✅ Edge cases: blocked patterns, empty inputs, invalid scores, etc.
Architecture
- ✅
TutorAgentorchestrates lesson generation and Q&A - ✅
InteractiveTutorprovides menu-driven CLI experience - ✅
SQLiteStorehandles progress persistence - ✅
LessonLoaderToolmanages caching with fallback templates
Integration
- ✅ Clean integration with main CLI via
cortex tutor <package> - ✅ Supports: --list, --progress, --reset, --fresh, -q flags
- ✅ Documentation in docs/AI_TUTOR.md
Notes
- Requires Python 3.10+ (union type hints)
- Requires ANTHROPIC_API_KEY for LLM features
- Offline fallback lessons available for common packages (docker, git, nginx)
Ready for merge once Ansh approves.
Reviewed-by: Claude Code
Anshgrover23
left a comment
Anshgrover23
left a comment
There was a problem hiding this comment.
@srikrishnavansi Added some comments.
| from cortex.tutor.contracts import LessonResponse, QAResponse | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
| _client: anthropic.Anthropic | None = None |
There was a problem hiding this comment.
There is no proper thread-safe initialization (no locks), for e.g SQLiteStore uses RLock.
| ) | ||
|
|
||
| # Extract tool use result | ||
| for block in response.content: |
There was a problem hiding this comment.
What if response.content is empty or None ?
| def cache_lesson( | ||
| self, | ||
| package_name: str, | ||
| lesson: dict[str, Any], | ||
| ttl_hours: int = 24, | ||
| ) -> bool: | ||
| """Cache a lesson for future retrieval.""" |
There was a problem hiding this comment.
TTL uses default but isn't configurable through Config class.
- Make model name configurable via TUTOR_MODEL_NAME env variable - Add thread-safe lock to client initialization (double-checked locking) - Handle empty/None response.content in llm.py - Remove duplicate validation from cli.py (agent.py handles it) - Remove unused _description parameter from create_progress_bar - Add _error_response helper for consistent error structure - Rename DEFAULT_TUTOR_TOPICS to DEFAULT_TUTOR_TOPICS_COUNT - Make cache TTL configurable via TUTOR_CACHE_TTL_HOURS env variable - Update tests to work with new config-based approach
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cortex/tutor/tools.py`:
- Around line 88-97: The clear_cache method's docstring and name imply a full
purge but the current path for package_name=None only calls
self.store.clear_expired_cache; either update the docstring to state it only
clears expired entries or implement a true "clear all" branch: add logic in
clear_cache to call a store-level full clear (e.g., self.store.clear_all_cache()
or iterate over cached keys and remove them) when package_name is None, and keep
the existing package-specific path that uses
self.store.cache_lesson(package_name, {}, ttl_hours=0); ensure any new store
method name (clear_all_cache or similar) is referenced consistently in the
method and tests.
♻️ Duplicate comments (1)
cortex/tutor/agent.py (1)
320-335: Don’t mark the tutorial complete if the user quits early.The loop breaks on
'q', but completion is still recorded with a full score. Track completed steps and compute a proportional score (or skip marking if quit immediately).🔧 Proposed fix
- for step in steps: + completed_steps = 0 + for step in steps: print_tutorial_step( step.get("content", ""), step.get("step_number", 1), len(steps), ) @@ response = get_user_input("Press Enter to continue (or 'q' to quit)") if response.lower() == "q": break + completed_steps += 1 - self.agent.mark_completed(self.package_name, "tutorial", 0.9) + if completed_steps > 0: + score = 0.9 * (completed_steps / len(steps)) + self.agent.mark_completed(self.package_name, "tutorial", score)
🧹 Nitpick comments (4)
cortex/tutor/config.py (2)
53-56: Add a type hint for themodel_post_initcontext parameter.
__contextis untyped; adding a type hint keeps this aligned with the project’s “type hints required” rule.🔧 Proposed fix
+from typing import Any from pathlib import Path @@ - def model_post_init(self, __context) -> None: + def model_post_init(self, __context: Any) -> None:As per coding guidelines, type hints are required in Python code.
112-113: Avoid a mkdir race by usingexist_ok=True.A concurrent creator can trigger
FileExistsErrorbetween theexists()check andmkdir().🔧 Proposed fix
- self.data_dir.mkdir(parents=True, mode=0o700) + self.data_dir.mkdir(parents=True, mode=0o700, exist_ok=True)cortex/tutor/tools.py (1)
46-70: Log cache read/write failures to avoid silent errors.Exceptions are swallowed and only returned to callers; adding
logger.exceptionpreserves tracebacks for debugging.🔧 Proposed fix
except Exception as e: + logger.exception("Failed to load cached lesson for %s", package_name) return { "success": False, "cache_hit": False, "lesson": None, "error": str(e), } @@ - except Exception: + except Exception: + logger.exception("Failed to cache lesson for %s", package_name) return FalseAlso applies to: 79-86
cortex/tutor/llm.py (1)
39-48: Model-specific pricing should replace hardcoded Sonnet rates.
config.model_nameis configurable via environment variable, but cost calculation uses hardcoded Sonnet rates ($3.00 input, $15.00 output per 1M tokens). This means costs will be inaccurate for other Claude models (Opus, Haiku) or Sonnet 4's long-context tier (>200K input tokens, which costs $6.00/$22.50).A per-model rate map with sensible defaults would ensure cost accuracy across model variants:
Suggested implementation
-# Cost per 1M tokens for Claude Sonnet -COST_INPUT_PER_1M = 3.0 -COST_OUTPUT_PER_1M = 15.0 +# Cost per 1M tokens (input_rate, output_rate) by model +_COST_PER_1M_BY_MODEL: dict[str, tuple[float, float]] = { + "claude-sonnet-4-20250514": (3.0, 15.0), +} @@ -def _calculate_cost(input_tokens: int, output_tokens: int) -> float: +def _calculate_cost(input_tokens: int, output_tokens: int, model_name: str) -> float: """Calculate cost in USD.""" - input_cost = (input_tokens / 1_000_000) * COST_INPUT_PER_1M - output_cost = (output_tokens / 1_000_000) * COST_OUTPUT_PER_1M + input_rate, output_rate = _COST_PER_1M_BY_MODEL.get(model_name, (0.0, 0.0)) + input_cost = (input_tokens / 1_000_000) * input_rate + output_cost = (output_tokens / 1_000_000) * output_rate return input_cost + output_cost @@ - cost = _calculate_cost(response.usage.input_tokens, response.usage.output_tokens) + cost = _calculate_cost( + response.usage.input_tokens, + response.usage.output_tokens, + config.model_name, + )
|
Code Changes:
Documentation Updates: Updated docs/AI_TUTOR.md with new environment variables (TUTOR_MODEL_NAME, TUTOR_CACHE_TTL_HOURS) Ready for re-review! |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cortex/cli.py (1)
2352-2367: Add-v/--verboseflag to tutor subcommand parser for consistency.The tutor subcommand parser is missing the
--verbose/-vargument that is defined incortex/tutor/cli.pyand present in other subcommand parsers (benchmark, systemd, gpu, printer). This creates an inconsistency where users must usecortex -v tutor dockerinstead of the expectedcortex tutor -v docker. The delegation tocortex/tutor/cli.pyfunctions works, but the parser definition should match to provide consistent UX across all subcommands.
🤖 Fix all issues with AI agents
In `@cortex/tutor/tools.py`:
- Around line 64-70: The LessonLoaderTool._run currently catches all exceptions
and returns an error dict without logging; update LessonLoaderTool._run to log
the exception before returning (mirroring ProgressTrackerTool._run) by calling
the appropriate logger (e.g., self.logger.exception(...) or module/process
logger) with the exception and stack trace, then return the same error dict;
ensure you reference the LessonLoaderTool._run function and use logger.exception
or logger.error(..., exc_info=True) so the stack trace is recorded.
- Around line 85-86: The cache_lesson function currently swallows all exceptions
and returns False with no diagnostics; update its except block to catch
Exception as e and log the error and stacktrace (e.g., use logger.exception or
logging.exception) including contextual info like the lesson id or parameters
used, then continue to return False (or re-raise if desired) so failures are
visible in logs; locate the except block in cache_lesson and replace the bare
except with a logged exception that includes the exception object and relevant
context.
♻️ Duplicate comments (1)
cortex/tutor/tools.py (1)
449-461: Missing audit logging for_reset_progressoperation.Based on learnings, audit logging to
~/.cortex/history.dbis required for installation operations. The_reset_progressmethod performs a destructive operation (deleting progress records) but doesn't implement audit logging. This was flagged in a past review as a major issue.
🧹 Nitpick comments (6)
cortex/licensing.py (1)
204-218: Box alignment may break with variable-length content.The box uses fixed-width borders, but lines 209, 213, and 215 contain variable-length content (
feature_display,tier_display,price,PRICING_URL) that won't align with the closing│characters. Consider either:
- Using dynamic padding to align the closing borders
- Removing the right-side borders for lines with dynamic content
♻️ Suggested fix (remove right borders for dynamic lines)
print(f""" ┌─────────────────────────────────────────────────────────┐ │ ⚡ UPGRADE REQUIRED │ ├─────────────────────────────────────────────────────────┤ │ │ -│ '{feature_display}' requires Cortex {tier_display} +│ '{feature_display}' requires Cortex {tier_display} │ │ │ │ ✅ Upgrade now: cortex upgrade │ │ │ -│ Plans start at {price}/month with 14-day free trial. +│ Plans start at {price}/month with 14-day free trial. │ │ │ -│ 🌐 {PRICING_URL} +│ 🌐 {PRICING_URL:<42} │ │ │ └─────────────────────────────────────────────────────────┘ """)docs/AI_TUTOR.md (2)
91-117: Add language specifier to fenced code block.This ASCII art output example is missing a language specifier. Use
textorplaintextfor non-code output blocks to satisfy linting and improve accessibility.🔧 Suggested fix
-``` +```text ___ _ _ _ _ _ _____ _
249-303: Add language specifiers to architecture diagram code blocks.The ASCII architecture diagrams at lines 249, 309, 348, 390, and 432 are missing language specifiers. Use
textfor these blocks.🔧 Suggested fix (apply to each diagram block)
-``` +```text ┌─────────────────────────────────────────────────────────────────┐ │ User Request │Also applies to lines: 309-344, 348-386, 390-424, 432-444
cortex/tutor/config.py (2)
51-54: Consider using__context: Anytype hint for Pydantic compatibility.The
model_post_initmethod is correctly used for computed field initialization. However, the__contextparameter should be typed asAnyfor proper Pydantic compatibility and to satisfy type checkers.♻️ Suggested improvement
+from typing import Any + - def model_post_init(self, __context) -> None: + def model_post_init(self, __context: Any) -> None: """Initialize computed fields after model creation.""" if self.db_path is None: self.db_path = self.data_dir / "tutor_progress.db"
123-133: API key validation is minimal — consider documenting limitations.The
validate_api_key()method only checks thesk-ant-prefix. While this is a reasonable quick check, Anthropic API keys have additional format requirements. Consider documenting that this is a format check only, not a validity check against the API.📝 Suggested docstring enhancement
def validate_api_key(self) -> bool: """ - Validate that the API key is properly configured. + Validate that the API key has the expected format. + + This performs a basic format check (prefix validation) only. + It does not verify the key against the Anthropic API. Returns: - bool: True if API key is valid format, False otherwise. + bool: True if API key has valid format, False otherwise. """cortex/tutor/tools.py (1)
472-490: Convenience functions instantiate new tools on every call.Each convenience function (
get_learning_progress,mark_topic_completed,get_package_stats) creates a newProgressTrackerToolinstance, which in turn creates a newSQLiteStoreconnection. For high-frequency usage, consider caching or reusing instances.♻️ Optional: Add a module-level cached instance
# Module-level cached instance for convenience functions _cached_tracker: ProgressTrackerTool | None = None def _get_tracker() -> ProgressTrackerTool: """Get or create a cached ProgressTrackerTool instance.""" global _cached_tracker if _cached_tracker is None: _cached_tracker = ProgressTrackerTool() return _cached_tracker def get_learning_progress(package_name: str, topic: str) -> dict[str, Any] | None: """Get learning progress for a specific topic.""" result = _get_tracker()._run("get_progress", package_name=package_name, topic=topic) return result.get("progress")
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cortex/tutor/tools.py`:
- Around line 415-425: The current _update_profile method uses a truthiness
check for learning_style which prevents intentionally setting it to an empty
string; change the condition so it checks explicitly for None (i.e., use
"learning_style is not None") before assigning profile.learning_style, leaving
the calls to self.store.get_student_profile() and
self.store.update_student_profile(profile) unchanged so an empty string can
clear the field.
♻️ Duplicate comments (1)
cortex/tutor/tools.py (1)
223-234: Missing audit logging for write operations.Per coding guidelines, operations should be logged to
~/.cortex/history.dbfor audit purposes. This tool performs mutations (mark_completed,update_progress,reset,update_profile,add_mastered,add_weak) without any audit trail.Consider adding audit logging, especially for destructive operations like
reset_progress. Based on learnings, this is a compliance requirement.
🧹 Nitpick comments (4)
cortex/tutor/tools.py (4)
100-105: Silent exception catch inclear_cachelacks logging.The
except Exceptionblock silently returns0without logging, inconsistent with the logging added to_run(line 65) andcache_lesson(line 87). This makes debugging cache clearing failures difficult.Suggested fix
if package_name: try: self.store.cache_lesson(package_name, {}, ttl_hours=0) return 1 - except Exception: + except Exception: + logger.exception("Failed to clear cache for package '%s'", package_name) return 0
188-216: Consider exposing a public interface instead of calling private method_run.
load_lesson_with_fallbackcallsloader._run()(line 194), but the underscore prefix conventionally indicates a private method. This creates a fragile dependency on internal implementation details.Suggested approach
Either rename
_runtorun(making it explicitly public), or add a public method toLessonLoaderTool:def load(self, package_name: str, force_fresh: bool = False) -> dict[str, Any]: """Load cached lesson content (public interface).""" return self._run(package_name, force_fresh)
427-449: Confusing dual parameter extraction pattern.The pattern
concept = kwargs.get("concept") or concept(lines 433, 445) allowsconceptto be passed both as a direct parameter and insidekwargs. This is confusing and has subtle bugs: ifconcept=""is explicitly passed butkwargs["concept"]has a value,kwargswins due to theoroperator's truthiness behavior.Suggested fix
Pick one source of truth. Since
_runpasses kwargs through, prefer extracting from kwargs only:def _add_mastered_concept( self, - concept: str | None = None, - **kwargs: Any, + **kwargs: Any, ) -> dict[str, Any]: """Add a mastered concept to student profile.""" - concept = kwargs.get("concept") or concept + concept = kwargs.get("concept") if not concept: return {"success": False, "error": "concept required"}Or, document that
conceptin kwargs takes precedence and use explicitNonecheck:- concept = kwargs.get("concept") or concept + concept = kwargs.get("concept") if "concept" in kwargs else concept
474-492: Convenience functions create new instances and connections on each call.Each convenience function instantiates a new
ProgressTrackerTool, which creates a newSQLiteStoreand database connection. This is acceptable for one-off CLI usage but inefficient if called repeatedly (e.g., in a loop or interactive session).Additionally, these call
tool._run(), which follows the private method anti-pattern noted earlier.Consider either:
- Using a module-level cached instance (lazy singleton), or
- Documenting that these are for one-off usage and providing the class directly for batch operations.
- Change 'if learning_style:' to 'if learning_style is not None:' to allow explicitly clearing learning_style with empty string - Add logger.exception() to clear_cache exception handler
|
@Anshgrover23
Ready for re-review. |
|
3 participants
Summary
Add comprehensive AI-powered tutor for package education with:
cortex tutor <package>Related Issue
Closes cortexlinux/cortex-distro#30
Type of Change
Demo
Demo.mov
AI Disclosure
Used Claude (Anthropic) via Claude Code CLI for:
Testing
pytest tests/tutor/ -vcortex tutor docker --freshChecklist
Summary by CodeRabbit
New Features
UX / Branding
Persistence & Tools
Configuration & Validation
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.