Advanced Installation Intent Detection (Issue #53) #213
Conversation
WalkthroughIntroduces an intent-detection system for parsing user requests into actionable intents, determining if clarification is needed, optionally enhancing detection via LLM, building installation plans, and integrating with the CLI for interactive user guidance. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/CLI
participant Det as IntentDetector
participant Clar as Clarifier
participant Ctx as SessionContext
participant LLM as LLMIntentAgent<br/>(+ optional LLM)
participant Plan as InstallationPlanner
User->>Det: detect(text)
Det-->>User: intents[]
User->>Clar: needs_clarification(intents, text)
alt Clarification needed
Clar-->>User: clarification_prompt
User->>User: Answer clarification
Note over User: Re-detect intents<br/>from refined input
else No clarification
Clar-->>User: None
end
User->>Ctx: add_intents(intents)
User->>Ctx: set_gpu(gpu_name)
Ctx-->>User: stored
User->>LLM: process(text)
LLM->>LLM: enhance_intents_with_llm<br/>(text, intents)
Note over LLM: Optional: call LLM<br/>if API key available
LLM->>Ctx: save intents
LLM->>Plan: build_plan(intents)
Plan-->>LLM: plan[]
opt LLM available
LLM->>LLM: suggest_optimizations(text)
Note over LLM: Call LLM for suggestions
end
LLM-->>User: {plan, intents,<br/>suggestions, gpu}
User->>User: Display & confirm
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Areas requiring extra attention:
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (1)cortex/cli.py (5)
🔇 Additional comments (4)
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 |
|
Hi @mikejmorgan-ai, |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
src/test_clarifier.py (1)
4-12: Consider asserting the specific clarifying question.The test name suggests checking for GPU-missing scenario, but the assertion only verifies that a question exists. Consider asserting the expected question content to make the test more precise and self-documenting.
Apply this diff to make the test more specific:
question = c.needs_clarification(intents, text) - assert question is not None + assert question is not None + assert "ML frameworks" in question # Should ask which frameworks are neededsrc/intent/planner.py (1)
19-42: Add defensive handling for unexpected install targets.The installation step loop handles known targets but silently adds unknown targets to the
installedset without generating a plan step. While current detector targets are covered, this creates tight coupling and could hide bugs if new targets are added.Apply this diff to add defensive logging or warning:
elif intent.target == "gpu": # GPU setup is handled by CUDA/cuDNN pass + else: + # Log or warn about unexpected target + print(f"Warning: Unknown install target '{intent.target}' - skipping") + continue # Don't add to installed set installed.add(intent.target)This prevents unknown targets from being marked as installed and makes debugging easier.
src/intent/llm_agent.py (3)
3-3: Remove unused import.The
osmodule is imported but never used in this file.Apply this diff:
-import os - # -------------------------------
114-114: Remove redundant import.The
Intentclass is already accessible via the module-level importfrom intent.detector import IntentDetector. Importing it again inside the method is unnecessary.Move the import to the top of the file:
from intent.detector import IntentDetector +from intent.detector import Intent from intent.planner import InstallationPlannerThen remove line 114:
- from intent.detector import Intent new_intents = intents[:]
117-126: Consider deduplicating intents before returning.The method appends new intents from the LLM without checking for duplicates. If the LLM suggests intents that were already detected by the rule-based system, they'll be added again.
Add deduplication logic:
for line in llm_output: if "install:" in line: pkg = line.replace("install:", "").strip() - new_intents.append(Intent("install", pkg)) + if not any(i.action == "install" and i.target == pkg for i in new_intents): + new_intents.append(Intent("install", pkg)) elif "configure:" in line: target = line.replace("configure:", "").strip() - new_intents.append(Intent("configure", target)) + if not any(i.action == "configure" and i.target == target for i in new_intents): + new_intents.append(Intent("configure", target)) elif "verify:" in line: target = line.replace("verify:", "").strip() - new_intents.append(Intent("verify", target)) + if not any(i.action == "verify" and i.target == target for i in new_intents): + new_intents.append(Intent("verify", target))src/intent/context.py (2)
33-34: Consider deduplicating intents to prevent accumulation.The
extendmethod will add duplicate intents ifadd_intentsis called multiple times with overlapping intents. Depending on how the context is used, this could lead to duplicate entries in the installation plan.Add deduplication logic:
def add_intents(self, intents: List[Intent]): - self.previous_intents.extend(intents) + for intent in intents: + # Check if intent already exists (by action and target) + if not any(i.action == intent.action and i.target == intent.target + for i in self.previous_intents): + self.previous_intents.append(intent)
54-55: Consider preventing duplicate clarification questions.Unlike
add_installedwhich checks for duplicates,add_clarificationwill append the same question multiple times if called repeatedly. While less critical than intent duplication, this could lead to a confusing user experience.Add a duplicate check:
def add_clarification(self, question: str): - self.clarifications.append(question) + if question not in self.clarifications: + self.clarifications.append(question)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.gitignore(0 hunks)src/intent/clarifier.py(1 hunks)src/intent/context.py(1 hunks)src/intent/detector.py(1 hunks)src/intent/llm_agent.py(1 hunks)src/intent/planner.py(1 hunks)src/test_clarifier.py(1 hunks)src/test_context.py(1 hunks)src/test_intent_detection.py(1 hunks)src/test_llm_agent.py(1 hunks)src/test_planner.py(1 hunks)
💤 Files with no reviewable changes (1)
- .gitignore
🧰 Additional context used
🧬 Code graph analysis (1)
src/intent/llm_agent.py (1)
src/intent/detector.py (3)
IntentDetector(12-49)detect(26-49)Intent(7-10)
🪛 Ruff (0.14.6)
src/intent/planner.py
8-8: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
src/test_llm_agent.py
4-4: Unused method argument: kwargs
(ARG002)
8-8: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
22-22: Unused lambda argument: a
(ARG005)
22-22: Unused lambda argument: k
(ARG005)
🔇 Additional comments (14)
src/test_context.py (1)
4-13: LGTM!The test correctly validates SessionContext storage and retrieval for GPU information, intents, and installed packages. The test coverage is appropriate for the basic storage operations.
src/test_intent_detection.py (2)
3-10: LGTM!The test correctly validates intent detection for a typical installation request. The use of set comprehension for target extraction is clean and appropriate.
12-15: LGTM!Good negative test case ensuring that unrelated text doesn't trigger false positive detections.
src/test_planner.py (1)
4-16: LGTM!The test validates end-to-end planning for GPU-enabled ML installation. The assertions correctly verify that CUDA, PyTorch, and GPU configuration steps are included in the proper order with verification at the end.
src/test_llm_agent.py (1)
15-28: LGTM!The test correctly mocks the LLM integration and validates the agent's process flow. The mock pattern is appropriate for testing LLM-dependent code without external API calls. The unused lambda arguments on line 22 are intentional for disabling clarification during testing.
src/intent/clarifier.py (1)
12-33: LGTM!The clarification rules are well-designed and handle common ambiguities in ML installation requests. The rule-based approach with early return ensures users get one clarifying question at a time, which is good UX. The logic correctly identifies scenarios where additional information is needed.
src/intent/planner.py (1)
44-50: LGTM!The final configuration and verification steps are correctly implemented. GPU configuration is conditionally added based on GPU-related intents, and verification is always included as the final step.
src/intent/detector.py (1)
26-49: LGTM!The detection logic is well-implemented with appropriate case-insensitive matching, deduplication for GPU configuration intents, and clear separation between package installation and GPU configuration. The rule-based approach correctly handles the most common ML installation scenarios.
src/intent/context.py (4)
6-17: LGTM! Clean session context initialization.The class provides a clear structure for storing session state across user interactions. The fields (GPU info, intents, packages, clarifications) align well with the PR objectives for context-aware intent detection.
23-27: LGTM! GPU context methods are straightforward.The simple getter/setter pattern is appropriate for this use case.
43-48: LGTM! Package tracking properly prevents duplicates.The implementation correctly checks for duplicates before adding packages (line 44) and provides a clean membership check.
64-69: LGTM! Clean reset implementation.The reset method properly reinitializes all session state fields, enabling clean session boundaries.
src/intent/llm_agent.py (2)
100-100: Unfortunately, I cannot proceed with the verification task because the repository failed to clone. Without access to the codebase, I'm unable to search for the Intent class definition or verify whether it has__str__or__repr__methods.To complete this verification, I need:
- Access to the repository
cortexlinux/cortex- Ability to inspect the Intent class implementation
Could you please check if:
- The repository is accessible and public
- There are any network or authentication issues preventing the clone
Alternatively, if you have access to the code, please verify the Intent class definition and share whether it includes
__str__or__repr__methods.
29-30: I'm unable to clone the repository to verify the Python version requirement. However, based on the code snippet provided and technical knowledge, I can confirm the following:The union type syntax
str | Nonerequires Python 3.10+. This is a technical fact regardless of project configuration. The original review comment accurately identifies this potential issue.Since I cannot access the repository's configuration files (pyproject.toml, setup.py, setup.cfg, .python-version, or CI/CD workflows) to determine the project's actual Python version target, I'll preserve the original review comment as-is:
Verify Python version requirement for union type syntax.
The
str | Noneunion syntax requires Python 3.10+. If the project supports Python 3.9 or earlier, useOptional[str]instead.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/intent/llm_agent.py (5)
70-73: Add logging for LLM failures to aid debugging.The exception handlers correctly provide safe fallbacks, but silently swallowing exceptions makes debugging difficult when LLM calls fail. Consider logging the exception with context before falling back.
Apply this pattern to both exception handlers:
# 4. Improve intents using LLM (safe) try: improved_intents = self.enhance_intents_with_llm(text, intents) - except Exception: + except Exception as e: + # Log error and fall back to original intents + # Example: logger.warning(f"LLM enhancement failed: {e}") improved_intents = intents # ... # 6. Optional suggestions from LLM (safe) try: suggestions = self.suggest_optimizations(text) - except Exception: + except Exception as e: + # Log error and fall back to empty suggestions + # Example: logger.warning(f"LLM suggestions failed: {e}") suggestions = []Note: You'll need to add a logger at the module level (e.g.,
import logging; logger = logging.getLogger(__name__)).Also applies to: 82-85
48-92: Consider adding return type hints for better type safety.The
processmethod would benefit from a return type hint to document the expected structure and improve IDE support.- def process(self, text: str): + def process(self, text: str) -> dict:Similarly, consider adding type hints for the dictionary structure using
TypedDictfor even better type safety.
123-123: Move import to module level for better practice.Importing
Intentinside the method is unconventional. While Python caches imports, it's clearer and more maintainable to import at the top of the file alongside theIntentDetectorimport.Move this import to the top of the file:
from intent.detector import IntentDetector +from intent.detector import Intent from intent.planner import InstallationPlannerThen remove line 123 inside the method.
97-137: Consider adding return type hints.The method would benefit from a return type hint for better documentation and type safety.
- def enhance_intents_with_llm(self, text: str, intents): + def enhance_intents_with_llm(self, text: str, intents: list) -> list:
142-162: Consider adding return type hints.The method would benefit from a return type hint for better documentation.
- def suggest_optimizations(self, text: str): + def suggest_optimizations(self, text: str) -> list[str]:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/intent/llm_agent.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/intent/llm_agent.py (1)
src/intent/detector.py (3)
IntentDetector(12-49)detect(26-49)Intent(7-10)
🪛 Ruff (0.14.6)
src/intent/llm_agent.py
72-72: Do not catch blind exception: Exception
(BLE001)
84-84: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (2)
src/intent/llm_agent.py (2)
1-16: LGTM! Safe import pattern for optional dependency.The safe import of the Anthropic SDK with a fallback to
Noneallows the agent to function in degraded mode when the SDK is unavailable, which is appropriate for CI environments and testing scenarios.
19-43: LGTM! Clean initialization with proper fallback handling.The constructor correctly initializes all components and safely handles cases where the Anthropic SDK or API key is unavailable by setting
self.llmtoNone, enabling graceful degradation.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/intent/llm_agent.py (2)
29-44: Consider adding type hints for better code clarity.While the code works correctly, adding type hints would improve IDE support and make the code more maintainable.
Apply this diff:
- def __init__(self, api_key: str | None = None, - model: str = "claude-3-5-sonnet-20240620"): + def __init__(self, api_key: str | None = None, + model: str = "claude-3-5-sonnet-20240620") -> None:
69-85: Add logging to exception handlers for better debugging.While the broad exception handling provides safe fallback behavior, silently swallowing exceptions makes debugging difficult. Consider adding logging to capture and surface errors while still maintaining graceful degradation.
Apply this diff:
+import logging + +logger = logging.getLogger(__name__) + # ... (in the process method) # 4. Improve intents using LLM (safe) try: improved_intents = self.enhance_intents_with_llm(text, intents) - except Exception: + except Exception as e: + logger.warning(f"LLM intent enhancement failed: {e}") improved_intents = intents # Save them to context self.context.add_intents(improved_intents) # 5. Build installation plan plan = self.planner.build_plan(improved_intents) # 6. Optional suggestions from LLM (safe) try: suggestions = self.suggest_optimizations(text) - except Exception: + except Exception as e: + logger.warning(f"LLM optimization suggestions failed: {e}") suggestions = []
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/intent/llm_agent.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/intent/llm_agent.py (1)
src/intent/detector.py (3)
IntentDetector(12-49)detect(26-49)Intent(7-10)
🪛 Ruff (0.14.6)
src/intent/llm_agent.py
72-72: Do not catch blind exception: Exception
(BLE001)
84-84: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (1)
src/intent/llm_agent.py (1)
142-162: LGTM! Timeout and safety checks properly implemented.This method correctly implements the timeout via
with_options(timeout=30.0)and includes proper safety checks for the response structure. The implementation aligns with the recommendations from past reviews.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/intent/llm_agent.py (3)
66-83: Broader-than-necessary exception handling; consider documenting or narrowingBoth LLM call sites in
processswallow allExceptionwithout logging, which (a) triggers Ruff BLE001 and (b) can hide genuine programming errors, not just transient API/network failures.If the intent is to be maximally defensive here, I’d at least make that explicit and keep Ruff quiet:
- try: - improved_intents = self.enhance_intents_with_llm(text, intents) - except Exception: - improved_intents = intents + try: + improved_intents = self.enhance_intents_with_llm(text, intents) + except Exception: # noqa: BLE001 - safe fallback if LLM fails + improved_intents = intents @@ - try: - suggestions = self.suggest_optimizations(text) - except Exception: - suggestions = [] + try: + suggestions = self.suggest_optimizations(text) + except Exception: # noqa: BLE001 - suggestions are strictly optional + suggestions = []Alternatively, you could narrow these to the specific Anthropic/client/network exception types you expect and keep real coding errors visible.
94-137: Guard against duplicate intents when merging LLM output
enhance_intents_with_llmunconditionally appends any parsedinstall:/configure:/verify:lines tonew_intents. If the LLM echoes existing intents (or repeats the same one multiple times), downstream code may see duplicates unless the planner handles deduping.You could cheaply dedupe here while preserving order:
- new_intents = intents[:] - - for line in llm_output: + new_intents = intents[:] + seen = {(i.action, i.target) for i in new_intents} + + for line in llm_output: if "install:" in line: pkg = line.replace("install:", "").strip() - if pkg: - new_intents.append(Intent("install", pkg)) + if pkg: + key = ("install", pkg) + if key not in seen: + new_intents.append(Intent("install", pkg)) + seen.add(key) elif "configure:" in line: target = line.replace("configure:", "").strip() - if target: - new_intents.append(Intent("configure", target)) + if target: + key = ("configure", target) + if key not in seen: + new_intents.append(Intent("configure", target)) + seen.add(key) elif "verify:" in line: target = line.replace("verify:", "").strip() - if target: - new_intents.append(Intent("verify", target)) + if target: + key = ("verify", target) + if key not in seen: + new_intents.append(Intent("verify", target)) + seen.add(key)If the planner already handles duplicates centrally, feel free to ignore this, but then it might be worth adding a brief comment here noting that assumption.
108-113: Timeout handling is consistent; consider centralizing the valueBoth LLM calls use
with_options(timeout=30.0), which is good for avoiding long hangs. To make this easier to tune and keep consistent, you could hoist30.0to a constructor argument or module-level constant (e.g.,DEFAULT_LLM_TIMEOUT = 30.0) and reference that in both places.Also applies to: 152-156
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/intent/llm_agent.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/intent/llm_agent.py (1)
src/intent/detector.py (3)
IntentDetector(12-49)Intent(7-10)detect(26-49)
🪛 Ruff (0.14.6)
src/intent/llm_agent.py
69-69: Do not catch blind exception: Exception
(BLE001)
81-81: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (2)
src/intent/llm_agent.py (2)
6-14: Safe Anthropic import and LLM gating look solidThe conditional Anthropic import plus
api_keycheck cleanly disables LLM usage and routes callers through the deterministic fallback path, while wiring the detector/planner/clarifier/context in__init__keeps responsibilities well-separated. No changes needed here.Also applies to: 27-42
142-162: Suggestion parsing is straightforward and robustThe
suggest_optimizationsprompt and parsing logic are simple and defensive (timeout + content shape check + stripping empty lines). For the intended use in the planner UI, this looks clean and sufficient.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
requirements.txt (1)
9-9: Specify an explicit version constraint for PyYAML.PyYAML is added without a version constraint, which breaks the consistency of the existing dependencies and introduces a reproducibility risk. All other dependencies use explicit versions (e.g.,
anthropic>=0.18.0).Update line 9 to include a version constraint:
-PyYAML +PyYAML>=6.0Consider checking the minimum PyYAML version required by your codebase and adjust accordingly. If you're targeting recent stability,
>=6.0is a reasonable choice; otherwise, use the version your intent modules were tested with.
|
Hi @mikejmorgan-ai, all CI checks now pass after adding PyYAML to requirements. |
|
Hi Mike, the CodeQL failure seems to come from the repository’s default setup vs. custom CodeQL workflow conflict. |
|
@Sahilbhatane Could you review this PR? Your multi-step coordinator experience would be helpful here. Thanks! |
Sahilbhatane
left a comment
Sahilbhatane
left a comment
There was a problem hiding this comment.
main branch is updated, rebase this PR, check for any conflicts and solve them
…e feature with intents, planner, clarifier, LLM agent, context, tests, and documentation
…tents in offline mode
pavanimanchala53
force-pushed
the
feature/advanced-intent-detection
branch
from
9cb5b17 to
4277225
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/intent/planner.py (1)
3-51: Planner logic looks solid; tidy upGPU_PACKAGESdefinition.The plan construction (GPU detection, deduped per‑package install steps, optional GPU configuration, and final verification) is clear and matches the tests. The remaining nit is the class‑level
GPU_PACKAGESlist, which is both mutable and unused right now. To satisfy Ruff and make intent explicit, either annotate it as a class-level constant or drop it if you don’t plan to use it:-from typing import List +from typing import ClassVar, List from intent.detector import Intent class InstallationPlanner: - GPU_PACKAGES = ["cuda", "cudnn", "pytorch", "tensorflow"] + GPU_PACKAGES: ClassVar[List[str]] = ["cuda", "cudnn", "pytorch", "tensorflow"]If
GPU_PACKAGESisn’t referenced elsewhere, consider removing it entirely to avoid confusion.src/test_llm_agent.py (1)
1-28: Mock LLM only exercises the error/fallback path; make it mirror the real client and clean up static-analysis nits.Right now
LLMIntentAgentcallsself.llm.with_options(...).messages.create(...), butMockLLMdoesn’t implementwith_options, so both LLM calls raiseAttributeErrorand get swallowed by the broadexcept Exception. That means this test never validates the successful LLM path or the parsing of the mock text; it only checks that the fallback behavior works. The class-levelcontentlist and unused arguments also trip Ruff.You can address all of this by making the mock match the Anthropic-style interface more closely and fixing the unused-arg patterns:
-from intent.llm_agent import LLMIntentAgent - -class MockMessages: - def create(self, **kwargs): - class Response: - class Content: - text = "install: tensorflow\ninstall: jupyter" - content = [Content()] - return Response() - -class MockLLM: - def __init__(self): - self.messages = MockMessages() - -def test_llm_agent_mocked(): - agent = LLMIntentAgent(api_key="fake-key") +from intent.llm_agent import LLMIntentAgent + + +class MockMessages: + def create(self, **_kwargs): + class Response: + class Content: + text = "install: tensorflow\ninstall: jupyter" + + def __init__(self): + # Match response.content[0].text access pattern + self.content = [self.Content()] + + return Response() + + +class MockLLM: + def __init__(self): + self.messages = MockMessages() + + def with_options(self, **_kwargs): + # Mirror the Anthropic client interface used by LLMIntentAgent + return self + + +def test_llm_agent_mocked(): + agent = LLMIntentAgent(api_key="fake-key") @@ - # Disable clarification during testing - agent.clarifier.needs_clarification = lambda *a, **k: None + # Disable clarification during testing + agent.clarifier.needs_clarification = lambda *_args, **_kwargs: NoneWith this change, the test will actually exercise the successful LLM code paths while also resolving the Ruff warnings (ARG002, RUF012, ARG005).
🧹 Nitpick comments (1)
src/intent/llm_agent.py (1)
27-89: Orchestration and safety checks look good; consider improving exception handling visibility.The overall flow (detector → clarifier → optional LLM refinement → planning → optional suggestions) is well-structured, and the response-shape checks in
enhance_intents_with_llm/suggest_optimizationsgive you a safe fallback if the LLM behaves unexpectedly. The only concern is the broadexcept Exceptionblocks around both LLM calls: they prevent crashes (which is good for a CLI) but also silently mask programming bugs and library misconfigurations.It would be safer to at least log the exception (and ideally narrow the caught types to Anthropic/httpx‑related errors), while still falling back to the rule‑based intents and empty suggestions:
- try: - improved_intents = self.enhance_intents_with_llm(text, intents) - except Exception: - improved_intents = intents + try: + improved_intents = self.enhance_intents_with_llm(text, intents) + except Exception as exc: + # TODO: plug into your logging/telemetry; keep UX-safe fallback + improved_intents = intents @@ - try: - suggestions = self.suggest_optimizations(text) - except Exception: - suggestions = [] + try: + suggestions = self.suggest_optimizations(text) + except Exception as exc: + # TODO: log `exc` for observability; keep UX-safe fallback + suggestions = []This preserves the “never crash on LLM issues” guarantee while making failures debuggable and avoiding completely silent degradation of the smart path.
Also applies to: 94-162
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.gitignore(0 hunks)requirements.txt(1 hunks)src/intent/clarifier.py(1 hunks)src/intent/context.py(1 hunks)src/intent/detector.py(1 hunks)src/intent/llm_agent.py(1 hunks)src/intent/planner.py(1 hunks)src/test_clarifier.py(1 hunks)src/test_context.py(1 hunks)src/test_intent_detection.py(1 hunks)src/test_llm_agent.py(1 hunks)src/test_planner.py(1 hunks)
💤 Files with no reviewable changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (4)
- src/test_intent_detection.py
- src/intent/context.py
- src/test_clarifier.py
- src/test_context.py
🧰 Additional context used
🧬 Code graph analysis (1)
src/intent/llm_agent.py (1)
src/intent/detector.py (2)
Intent(7-10)detect(26-49)
🪛 Ruff (0.14.8)
src/test_llm_agent.py
4-4: Unused method argument: kwargs
(ARG002)
8-8: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
22-22: Unused lambda argument: a
(ARG005)
22-22: Unused lambda argument: k
(ARG005)
src/intent/llm_agent.py
69-69: Do not catch blind exception: Exception
(BLE001)
81-81: Do not catch blind exception: Exception
(BLE001)
src/intent/planner.py
8-8: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
🔇 Additional comments (3)
src/intent/detector.py (1)
6-49: Intent detection pipeline looks correct and cohesive.Centralizing keyword synonyms in
COMMON_PACKAGES, lowercasing once, and separating GPU configuration from install intents aligns well with the clarifier and planner; the rule-based extraction is clear and deterministic, and I don’t see functional issues here.src/test_planner.py (1)
4-16: Good end-to-end coverage for CUDA/GPU planning.This test exercises the typical CUDA + PyTorch + GPU configuration flow and asserts the critical plan entries (including the final verification step), which should catch most regressions in
InstallationPlanner.build_plan.src/intent/clarifier.py (1)
6-33: Clarification heuristics are reasonable and integrate cleanly.The checks for ambiguous GPU usage, generic “ML tools” requests, CUDA-without-GPU, and PyTorch version ambiguity are straightforward and side‑effect free, and they plug neatly into the main agent flow without risking spurious clarifications. Good foundation for future context-aware refinements.
pavanimanchala53
force-pushed
the
feature/advanced-intent-detection
branch
from
091cf99 to
4277225
Compare
pavanimanchala53
force-pushed
the
feature/advanced-intent-detection
branch
from
481b367 to
4277225
Compare
|
@pavanimanchala53 let it be, the more you commit the more I have to compare. I will solve rest of the errors |
Fix Issue cortexlinux#53: Add interactive clarification flow and user confirmation
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/intent/planner.py (1)
8-8: Annotate GPU_PACKAGES with ClassVar.The
GPU_PACKAGESclass attribute should be annotated withClassVar[List[str]]to indicate it's not an instance variable. This issue was previously flagged and remains unresolved.Apply this diff:
-from typing import List +from typing import List, ClassVar from intent.detector import Intent class InstallationPlanner: - GPU_PACKAGES = ["cuda", "cudnn", "pytorch", "tensorflow"] + GPU_PACKAGES: ClassVar[List[str]] = ["cuda", "cudnn", "pytorch", "tensorflow"]Based on past review comments and static analysis hints.
🧹 Nitpick comments (9)
test/test_cli.py (5)
16-20: Verify that validate_api_key is called.The test mocks
validate_api_keybut doesn't verify it was invoked. Consider asserting the mock was called to ensure the test covers the expected code path.Apply this diff to verify the mock call:
@patch.dict(os.environ, {'OPENAI_API_KEY': 'sk-test-key'}) @patch('cortex.cli.validate_api_key', return_value=(True, 'openai', None)) def test_get_api_key_openai(self, mock_validate): api_key = self.cli._get_api_key() self.assertEqual(api_key, 'sk-test-key') + mock_validate.assert_called_once()
22-26: Verify that validate_api_key is called.Same as test_get_api_key_openai - the mock should be verified to ensure the code path is tested.
Apply this diff:
@patch.dict(os.environ, {'ANTHROPIC_API_KEY': 'sk-ant-test-key', 'OPENAI_API_KEY': ''}, clear=True) @patch('cortex.cli.validate_api_key', return_value=(True, 'claude', None)) def test_get_api_key_claude(self, mock_validate): api_key = self.cli._get_api_key() self.assertEqual(api_key, 'sk-ant-test-key') + mock_validate.assert_called_once()
66-77: Verify that validate_api_key is called.The test mocks
validate_api_keybut doesn't verify it was invoked.Apply this diff:
@patch.dict(os.environ, {'OPENAI_API_KEY': 'sk-test-key'}) @patch('cortex.cli.validate_api_key', return_value=(True, 'openai', None)) @patch('cortex.cli.CommandInterpreter') def test_install_dry_run(self, mock_interpreter_class, mock_validate): mock_interpreter = Mock() mock_interpreter.parse.return_value = ["apt update", "apt install docker"] mock_interpreter_class.return_value = mock_interpreter result = self.cli.install("docker", dry_run=True) self.assertEqual(result, 0) mock_interpreter.parse.assert_called_once_with("install docker") + mock_validate.assert_called_once()
79-91: Verify that mocks are called.Both
validate_api_keyandinputare mocked but not verified.Apply this diff:
@patch.dict(os.environ, {'OPENAI_API_KEY': 'sk-test-key'}) @patch('cortex.cli.validate_api_key', return_value=(True, 'openai', None)) @patch('builtins.input', return_value='n') @patch('cortex.cli.CommandInterpreter') def test_install_no_execute(self, mock_interpreter_class, mock_input, mock_validate): mock_interpreter = Mock() mock_interpreter.parse.return_value = ["apt update", "apt install docker"] mock_interpreter_class.return_value = mock_interpreter result = self.cli.install("docker", execute=False) self.assertEqual(result, 0) mock_interpreter.parse.assert_called_once() + mock_validate.assert_called_once()
93-112: Verify that validate_api_key is called.The mock should be verified to ensure proper test coverage.
Apply this diff:
@patch.dict(os.environ, {'OPENAI_API_KEY': 'sk-test-key'}) @patch('cortex.cli.validate_api_key', return_value=(True, 'openai', None)) @patch('cortex.cli.CommandInterpreter') @patch('cortex.cli.InstallationCoordinator') def test_install_with_execute_success(self, mock_coordinator_class, mock_interpreter_class, mock_validate): mock_interpreter = Mock() mock_interpreter.parse.return_value = ["echo test"] mock_interpreter_class.return_value = mock_interpreter mock_coordinator = Mock() mock_result = Mock() mock_result.success = True mock_result.total_duration = 1.5 mock_coordinator.execute.return_value = mock_result mock_coordinator_class.return_value = mock_coordinator result = self.cli.install("docker", execute=True) self.assertEqual(result, 0) mock_coordinator.execute.assert_called_once() + mock_validate.assert_called_once()cortex/cli.py (3)
153-198: Intent detection is skipped when --execute flag is used.The condition on Line 153 prevents intent detection from running when
execute=Trueordry_run=True. This means users who runcortex install docker --executewill skip the entire intent detection, clarification, and planning flow. While this may be intentional for power users, it creates an inconsistent experience.Consider running intent detection regardless of the execute flag, or document this behavior clearly for users.
190-194: Plan display condition inconsistent with comment.Line 190 only shows the plan if
len(plan) > 1, but the comment on Line 189 says "Always show plan if we have intents". Additionally, the conditionlen(plan) > 1means plans with exactly one step (e.g., just verification) won't be shown.Consider either:
- Removing the condition to always show the plan when intents are detected
- Updating the comment to match the actual behavior
- Using a more explicit condition like
if intents and plan:
216-239: Consider using a local variable instead of modifying the parameter.Line 231 modifies the
executeparameter directly (execute = True). While this works, it's clearer to use a separate local variable to track whether to proceed with execution.Apply this diff to improve clarity:
def install(self, software: str, execute: bool = False, dry_run: bool = False): # Validate input first is_valid, error = validate_install_request(software) if not is_valid: self._print_error(error) return 1 api_key = self._get_api_key() if not api_key: return 1 provider = self._get_provider() self._debug(f"Using provider: {provider}") self._debug(f"API key: {api_key[:10]}...{api_key[-4:]}") # Initialize installation history history = InstallationHistory() install_id = None start_time = datetime.now() + should_execute = execute try: # ... (intent detection code) # Show generated commands and ask for confirmation (Issue #53) if not execute and not dry_run: print("\nGenerated commands:") for i, cmd in enumerate(commands, 1): print(f" {i}. {cmd}") # Ask for confirmation before executing print() try: response = input("Proceed with plan? [Y/n]: ").strip().lower() if response == 'n' or response == 'no': cx_print("Installation cancelled by user.", "info") return 0 elif response == '' or response == 'y' or response == 'yes': # User confirmed, proceed with execution - execute = True + should_execute = True cx_print("\nProceeding with installation...", "success") else: cx_print("Invalid response. Installation cancelled.", "error") return 1 except (KeyboardInterrupt, EOFError): print("\n") cx_print("Installation cancelled by user.", "info") return 0 # Record installation start - if execute or dry_run: + if should_execute or dry_run: install_id = history.record_installation( InstallationType.INSTALL, packages, commands, start_time ) if not dry_run: self._print_status("⚙️", f"Installing {software}...") print("\nGenerated commands:") for i, cmd in enumerate(commands, 1): print(f" {i}. {cmd}") if dry_run: print("\n(Dry run mode - commands not executed)") if install_id: history.update_installation(install_id, InstallationStatus.SUCCESS) return 0 - if execute: + if should_execute: # ... (execution code)src/intent/planner.py (1)
23-52: Consider refactoring the if-elif chain to a mapping dict.The long if-elif chain for mapping targets to installation steps could be refactored to a dictionary for better maintainability and extensibility.
Here's an example refactoring:
class InstallationPlanner: GPU_PACKAGES: ClassVar[List[str]] = ["cuda", "cudnn", "pytorch", "tensorflow"] INSTALL_STEPS: ClassVar[dict[str, str]] = { "cuda": "Install CUDA 12.3 + drivers", "cudnn": "Install cuDNN (matching CUDA version)", "pytorch": "Install PyTorch (GPU support)", "tensorflow": "Install TensorFlow (GPU support)", "jupyter": "Install JupyterLab", "python": "Install Python 3", "docker": "Install Docker", "nodejs": "Install Node.js and npm", "git": "Install Git", } def build_plan(self, intents: List[Intent]) -> List[str]: plan = [] installed = set() # 1. If GPU-related intents exist → add GPU detection has_gpu = any(i.target in self.GPU_PACKAGES or i.target == "gpu" for i in intents) if has_gpu: plan.append("Detect GPU: Run `nvidia-smi` or PCI scan") # 2. Add installation steps based on intent order for intent in intents: if intent.action == "install" and intent.target not in installed: if intent.target in self.INSTALL_STEPS: plan.append(self.INSTALL_STEPS[intent.target]) installed.add(intent.target) elif intent.target == "gpu": # GPU setup is handled by CUDA/cuDNN installed.add(intent.target) # 3. Add GPU configuration if needed if has_gpu: plan.append("Configure GPU acceleration environment") # 4. Add verification step plan.append("Verify installation and GPU acceleration") return plan
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
cortex/cli.py(4 hunks)requirements-dev.txt(1 hunks)requirements.txt(1 hunks)src/intent/clarifier.py(1 hunks)src/intent/detector.py(1 hunks)src/intent/planner.py(1 hunks)test/test_cli.py(5 hunks)test/test_installation_history.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/intent/detector.py
- requirements.txt
- test/test_installation_history.py
- src/intent/clarifier.py
🧰 Additional context used
🧬 Code graph analysis (3)
test/test_cli.py (1)
cortex/cli.py (1)
_get_api_key(61-75)
cortex/cli.py (5)
src/intent/detector.py (2)
IntentDetector(12-53)detect(30-53)src/intent/planner.py (2)
InstallationPlanner(6-63)build_plan(10-63)src/intent/clarifier.py (2)
Clarifier(6-36)needs_clarification(12-36)cortex/coordinator.py (1)
execute(252-299)cortex/branding.py (1)
cx_print(55-75)
src/intent/planner.py (1)
src/intent/detector.py (1)
Intent(7-10)
🪛 Ruff (0.14.8)
test/test_cli.py
18-18: Unused method argument: mock_validate
(ARG002)
24-24: Unused method argument: mock_validate
(ARG002)
31-31: Unused method argument: mock_stderr
(ARG002)
31-31: Unused method argument: mock_validate
(ARG002)
69-69: Unused method argument: mock_validate
(ARG002)
83-83: Unused method argument: mock_input
(ARG002)
83-83: Unused method argument: mock_validate
(ARG002)
97-97: Unused method argument: mock_validate
(ARG002)
src/intent/planner.py
8-8: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
🔇 Additional comments (5)
test/test_cli.py (2)
179-182: LGTM!The change to return 0 (success) when no command is provided aligns with standard CLI behavior of showing help text.
28-34: Verify test expectation against current implementation.The test expects
'ollama-local'whenvalidate_api_keyreturns failure (line 34), but this needs to be confirmed against the actual current implementation of_get_api_key()incortex/cli.py. If the implementation still returnsNoneon validation failure rather than'ollama-local'as a fallback, the test expectation should be corrected toNone.cortex/cli.py (2)
16-19: LGTM!The sys.path manipulation correctly adds the src directory to enable intent module imports, with proper existence checking.
242-248: Verify that dry_run installations should be recorded in history.Line 242 records installations even when
dry_run=True. This means dry-run operations will appear in the installation history. Verify this is the intended behavior, as typically dry-run operations are not recorded since they don't actually modify the system.requirements-dev.txt (1)
23-23: PyYAML 6.0.3 is the latest version and includes all known security patches.PyYAML 6.0.3 (released September 25, 2025) is the current latest release and resolves all known CVEs including CVE-2017-18342, CVE-2019-20477, CVE-2020-1747, and CVE-2020-14343. The version selection is correct and secure.
Regarding placement: verify whether the intent detection modules that use PyYAML are production code or test-only utilities. If they are production code, PyYAML should be in requirements.txt rather than requirements-dev.txt.
|
@mikejmorgan-ai we can merge this PR, But issue was not MVP and was Closed. can you confirm it here as this is not a MVP issue and was closed earlier. |
@mikejmorgan-ai This PR also clears the test errors of automation. Merging this would allow automation to work properly |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
requirements-dev.txt(1 hunks)
🔇 Additional comments (2)
requirements-dev.txt (2)
4-4: pytest-mock addition looks good.The addition of
pytest-mock>=3.10.0appropriately supports the new test modules added in this PR (test_intent_detection.py, test_clarifier.py, test_context.py, test_llm_agent.py, test_planner.py) that mock external dependencies.
18-18: Verify PyYAML version specification strategy.Line 18 pins PyYAML to an exact version (
PyYAML==6.0.3) while other dependencies use minimum version constraints (>=). Confirm whether the exact pin is intentional (e.g., for reproducibility aligned with requirements.txt) or should match the>=pattern for consistency.
|
3 participants
Summary
This PR implements the full Advanced Installation Intent Detection system requested in Issue #53.
It adds a complete rule-based + context-aware pipeline for extracting, clarifying, and planning installation commands from natural language input.
All merge conflicts have been resolved after rebasing on the latest upstream
main.Features Added
Code Quality & Tests
mainFiles Added / Modified
src/intent/detector.pysrc/intent/clarifier.pysrc/intent/planner.pysrc/intent/context.pysrc/intent/llm_agent.pysrc/test_intent_detection.pysrc/test_clarifier.pysrc/test_planner.pysrc/test_context.pysrc/test_llm_agent.py.gitignore(merged and updated)Checklist
Thanks for reviewing!
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.