NL install: Improve prompt handling and reliability for demos #411
Conversation
pavanimanchala53
requested review from
Anshgrover23 and
mikejmorgan-ai
as code owners
CLA Verification PassedAll contributors have signed the CLA.
|
|
Warning Rate limit exceeded@pavanimanchala53 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 9 minutes and 45 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds intent extraction to the install flow, integrates stdin-aware prompt building, and introduces an interactive confirmation/editing step (with non-interactive auto-approve). It also expands LLM intent/command-parsing logic and adds a CLA signer entry. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as cortex/cli
participant Interp as CommandInterpreter
participant LLM as LLM Provider\n(OpenAI / Ollama)
participant Shell as System Shell
User->>CLI: run install <natural-language>
CLI->>CLI: _is_interactive() check
CLI->>CLI: _build_prompt_with_stdin(user_prompt)
CLI->>Interp: extract_intent(user_prompt)
activate Interp
Interp->>LLM: provider-specific intent request
activate LLM
LLM-->>Interp: intent JSON (action, install_mode, confidence)
deactivate LLM
Interp-->>CLI: structured intent
deactivate Interp
CLI->>Interp: generate commands (prompt w/ mode context)
activate Interp
Interp->>LLM: command-generation request
activate LLM
LLM-->>Interp: suggested commands
deactivate LLM
Interp-->>CLI: parsed command list
deactivate Interp
alt interactive
CLI->>User: show commands + [proceed / edit / cancel]
alt edit
User->>CLI: line-by-line edits
CLI->>User: show updated commands -> re-confirm
User->>CLI: confirm
else proceed
User->>CLI: confirm
end
else non-interactive
CLI->>CLI: auto-approve
end
CLI->>Shell: execute approved commands
Shell-->>CLI: execution result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cortex/llm/interpreter.py (1)
413-431: Unreachable code: lines 416–431 can never execute.The early return at lines 413–414 exits whenever
commandsis a list. Sincedata.get("commands", [])always returns a list (defaulting to[]), the code after line 414 handling object arrays is dead code.🔎 Proposed fix: integrate both parsing paths
data = json.loads(json_blob) commands = data.get("commands", []) - if isinstance(commands, list): - return [c for c in commands if isinstance(c, str) and c.strip()] - # Handle both formats: # 1. ["cmd1", "cmd2"] - direct string array # 2. [{"command": "cmd1"}, {"command": "cmd2"}] - object array + if not isinstance(commands, list): + raise ValueError("commands must be a list") + result = [] for cmd in commands: if isinstance(cmd, str): - # Direct string - if cmd: + if cmd.strip(): result.append(cmd) elif isinstance(cmd, dict): # Object with "command" key cmd_str = cmd.get("command", "") - if cmd_str: + if cmd_str and cmd_str.strip(): result.append(cmd_str) return result
🧹 Nitpick comments (4)
cortex/llm/interpreter.py (2)
144-158: Multiline string indentation introduces leading whitespace in prompt.The indentation inside the return string adds literal leading spaces to each line of the prompt sent to the LLM. This wastes tokens and may confuse some models. Consider using
textwrap.dedent()or removing the leading indentation.🔎 Proposed fix using dedent
+import textwrap + ... - return """You are a Linux system command expert. Convert natural language requests into safe, validated bash commands. - - Rules: - 1. Return ONLY a JSON array of commands + return textwrap.dedent("""\ + You are a Linux system command expert. Convert natural language requests into safe, validated bash commands. + + Rules: + 1. Return ONLY a JSON array of commands ... + """).strip()
528-560:_estimate_confidencemethod is never called — appears to be dead code.This method is defined but not invoked anywhere in the file. If it's intended for future use, consider adding a TODO comment or removing it to reduce maintenance burden.
cortex/cli.py (2)
655-656: Duplicate comment line.Lines 655 and 656 contain the same comment. Remove the duplicate.
# ---------- User confirmation ---------- - # ---------- User confirmation ----------
677-686: Edit mode UX could be clearer.The edit flow prompts with just
>without explaining how to finish input (empty line to end). Consider adding brief instructions.🔎 Proposed improvement
elif choice == "e": if not _is_interactive(): self._print_error("Cannot edit commands in non-interactive mode") return 1 + print("Enter commands (one per line, empty line to finish):") edited_commands = [] while True: line = input("> ").strip()
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cortex/cli.pycortex/llm/interpreter.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs
Files:
cortex/llm/interpreter.pycortex/cli.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 **/*install*.py : Dry-run by default for all installations in command execution
Applied to files:
cortex/cli.py
📚 Learning: 2025-12-11T12:03:24.071Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T12:03:24.071Z
Learning: Applies to **/*install*.py : No silent sudo execution - require explicit user confirmation
Applied to files:
cortex/cli.py
🧬 Code graph analysis (1)
cortex/cli.py (2)
cortex/first_run_wizard.py (1)
_print_error(746-748)cortex/llm/interpreter.py (2)
parse(458-516)extract_intent(562-573)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test (3.10)
- GitHub Check: test (3.12)
- GitHub Check: test (3.11)
🔇 Additional comments (3)
cortex/cli.py (3)
34-36: LGTM!Simple and correct implementation for detecting interactive mode.
611-633: Intent extraction integration looks correct.The integration of
extract_intentto determineinstall_modeand tailor the prompt is well-structured. Consider adding exception handling aroundextract_intentto provide a graceful fallback if intent extraction fails, similar to how command parsing failures are handled.
657-666: Non-interactive auto-approval may bypass user confirmation for sudo commands.Based on learnings, "No silent sudo execution - require explicit user confirmation" is expected. However, non-interactive mode (CI/pytest) auto-approves all commands including those with sudo. Consider logging a warning or ensuring the
--executeflag itself serves as the explicit confirmation in CI contexts.
|
here is the working video which shows installing commands using edit option and then proceeding with installation Screen.Recording.2026-01-01.160454.mp4 |
Anshgrover23
left a comment
Anshgrover23
left a comment
There was a problem hiding this comment.
@pavanimanchala53 Create a new PR following the pattern of this PR #401 , As currently CLA Verification CI check is failing.
pavanimanchala53
force-pushed
the
feature/nl-parser-clean
branch
from
64a2515 to
a6b8b49
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
cortex/cli.py (2)
44-53:self.stdin_datais still never initialized — stdin context feature remains non-functional.The method checks for
self.stdin_dataviagetattr, but this attribute is never assigned anywhere in the class or__init__. The stdin context prepending will always be skipped, making the feature dead code.Based on learnings, this should be initialized to capture stdin when available (e.g., in
__init__or before calling this method) by readingsys.stdin.read()whennot sys.stdin.isatty().
594-596: Hardcoded "docker installed successfully!" message is incorrect for non-docker software.This message is printed regardless of what software is being installed in fake mode. It should reflect the actual
softwareparameter.🔎 Proposed fix
if execute: - print("\ndocker installed successfully!") + print(f"\n{software} installed successfully!")cortex/llm/interpreter.py (4)
179-183:self.ollama_urlis undefined — this will raiseAttributeErrorat runtime.The
_extract_intent_ollamamethod referencesself.ollama_urlon line 180, but this attribute is never set. In_initialize_client(lines 93-122),ollama_base_urlis a local variable and is not stored on the instance.🔎 Proposed fix
Store the URL as an instance attribute in
_initialize_client:elif self.provider == APIProvider.OLLAMA: # Ollama uses OpenAI-compatible API try: from openai import OpenAI ollama_base_url = os.environ.get("OLLAMA_BASE_URL", "http://localhost:11434") + self.ollama_url = ollama_base_url self.client = OpenAI( api_key="ollama", base_url=f"{ollama_base_url}/v1" # Dummy key, not used )
231-238: Missing colon in JSON format example will confuse LLMs.Line 234 shows
"install_mode" "..."but should be"install_mode": "...". The LLM may produce malformed JSON responses that fail parsing.🔎 Proposed fix
Format: { "action": "...", "domain": "...", - "install_mode" "..." + "install_mode": "...", "description": "...", "ambiguous": true/false, "confidence": 0.0 }
258-270: Missing error handling in_extract_intent_openai— inconsistent with Ollama path.
_extract_intent_ollamareturns a fallback dict on failure (lines 191-199), but_extract_intent_openaiwill raise unhandled exceptions if the API call fails or returns malformed JSON. This creates inconsistent behavior between providers.🔎 Proposed fix: wrap in try/except with fallback
def _extract_intent_openai(self, user_input: str) -> dict: + try: response = self.client.chat.completions.create( model=self.model, messages=[ {"role": "system", "content": self._get_intent_prompt()}, {"role": "user", "content": user_input}, ], temperature=0.2, max_tokens=300, ) content = response.choices[0].message.content.strip() - return json.loads(content) + return self._parse_intent_from_text(content) + except Exception: + return { + "action": "unknown", + "domain": "unknown", + "install_mode": "system", + "description": "Failed to extract intent", + "ambiguous": True, + "confidence": 0.0, + }
562-573: Missing docstring for public APIextract_intentand FAKE provider not handled.Per coding guidelines, docstrings are required for all public APIs. Additionally, the
FAKEprovider is not handled and will raiseValueError, which may break tests or CLI paths that use the fake provider.🔎 Proposed fix
def extract_intent(self, user_input: str) -> dict: + """Extract user intent from natural language input. + + Args: + user_input: Natural language description of desired action + + Returns: + Dictionary containing action, domain, install_mode, description, + ambiguous flag, and confidence score. + + Raises: + ValueError: If input is empty or provider is unsupported + NotImplementedError: If provider does not support intent extraction + """ if not user_input or not user_input.strip(): raise ValueError("User input cannot be empty") if self.provider == APIProvider.OPENAI: return self._extract_intent_openai(user_input) elif self.provider == APIProvider.CLAUDE: raise NotImplementedError("Intent extraction not yet implemented for Claude") elif self.provider == APIProvider.OLLAMA: return self._extract_intent_ollama(user_input) + elif self.provider == APIProvider.FAKE: + # Return a default intent for testing + return { + "action": "install", + "domain": "unknown", + "install_mode": "system", + "description": "Fake intent for testing", + "ambiguous": False, + "confidence": 1.0, + } else: raise ValueError(f"Unsupported provider: {self.provider}")
🧹 Nitpick comments (2)
cortex/cli.py (2)
34-36: Consider adding type hints and a docstring for completeness.Per coding guidelines, type hints and docstrings are required for public APIs. While this is an internal helper, adding them improves maintainability.
🔎 Proposed enhancement
-def _is_interactive(): +def _is_interactive() -> bool: + """Check if stdin is connected to a TTY (interactive terminal). + + Returns: + bool: True if stdin is a TTY, False otherwise (e.g., in CI/piped input) + """ return sys.stdin.isatty()
656-656: Remove duplicate comment.The comment
# ---------- User confirmation ----------appears twice on consecutive lines.🔎 Proposed fix
- # ---------- User confirmation ---------- # ---------- User confirmation ---------- if execute:
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/cla-signers.jsoncortex/cli.pycortex/llm/interpreter.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs
Files:
cortex/llm/interpreter.pycortex/cli.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 **/*install*.py : Dry-run by default for all installations in command execution
Applied to files:
cortex/cli.py
📚 Learning: 2025-12-11T12:03:24.071Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T12:03:24.071Z
Learning: Applies to **/*install*.py : No silent sudo execution - require explicit user confirmation
Applied to files:
cortex/cli.py
🧬 Code graph analysis (1)
cortex/cli.py (1)
cortex/llm/interpreter.py (3)
CommandInterpreter(18-573)parse(458-516)extract_intent(562-573)
🔇 Additional comments (6)
.github/cla-signers.json (2)
26-34: LGTM! CLA entry structure is correct.The new CLA signer entry for Swaroop Manchala is well-formed, follows the established structure, and includes all required fields with valid data.
1-98: Verify: CLA update appears inconsistent with PR objectives.The PR objectives and AI summary describe substantial changes to the natural language install flow (intent extraction, stdin-aware prompt building, interactive confirmation/editing), but only the CLA signers file is provided for review.
Please confirm:
- Are the actual code changes for NL install improvements in a different file set?
- Is this CLA update for a co-contributor on this PR?
cortex/cli.py (1)
611-633: LGTM — Intent-driven prompt construction is well-structured.The extraction of
install_modefrom intent and the conditional prompt building (python vs system) provides good separation between Python virtualenv workflows and system package installation. The integration with_build_prompt_with_stdinallows for future stdin context enrichment once that feature is properly initialized.cortex/llm/interpreter.py (3)
144-157: LGTM — System prompt improvements enhance command generation clarity.The updated rules and structured format with examples should improve consistency across providers.
272-300: LGTM — Robust intent parsing with sensible fallback.The JSON extraction logic handles loose LLM output gracefully, validates required fields, and returns a safe default on failure. This approach prevents crashes from malformed responses.
528-560: LGTM — Confidence estimation uses reasonable heuristics.The linguistic signal approach (length, install keywords, vague words, domain) provides a practical confidence score without hardcoding domain-specific semantics. The clamping to [0.2, 1.0] ensures meaningful scores.
pavanimanchala53
force-pushed
the
feature/nl-parser-clean
branch
2 times, most recently
from
06b3928 to
07a4d5f
Compare
pavanimanchala53
force-pushed
the
feature/nl-parser-clean
branch
from
07a4d5f to
47ab94e
Compare
|
|
can you review this CodeQL is failing due to a repository configuration conflict |
Anshgrover23
left a comment
Anshgrover23
left a comment
There was a problem hiding this comment.
@pavanimanchala53 Follow Contributing.md guideliness i.e add a demonstration video.
There was a problem hiding this comment.
@pavanimanchala53 Missing test, docs, video. Do not attempt low quality PR reminding you again.
@mikejmorgan-ai I'm closing this one.
Feel free to open a new with all the changes,
2 participants
Related Issue
Closes #248
Summary
This PR improves the stability and reliability of the natural-language
install flow by refining prompt construction and handling edge cases
that affected demos and tests.
No hardcoded request mappings or fixed install logic were introduced.
What changed
Summary by CodeRabbit
New Features
Improvements
Style
Chores
✏️ Tip: You can customize this high-level summary in your review settings.