feat: Added voice feature for end-user fix #325 #405
Conversation
|
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 Linux push-to-talk voice input subsystem with faster-whisper, integrates voice/mic flows into the CLI, extends install to support parallel/sequential execution and JSON output, adjusts platform-aware terminal rendering, updates API key detection for explicit providers, and adds docs/tests and optional voice extras. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as CortexCLI
participant Voice as VoiceInputHandler
participant Mic as AudioDevice
participant Model as faster-whisper
participant Exec as CommandExecutor
User->>CLI: invoke "voice" or "ask --mic"
CLI->>Voice: get_voice_handler()
Voice->>Voice: ensure dependencies & check microphone
Voice->>Mic: open stream / wait for hotkey
Note over Voice,User: User presses hotkey (F9)
Mic-->>Voice: audio frames
Voice->>Model: transcribe audio
Model-->>Voice: text
Voice-->>CLI: transcribed text
CLI->>Exec: process command
Exec-->>CLI: result/status
CLI-->>User: output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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.
|
There was a problem hiding this comment.
Pull request overview
This PR adds voice input capability to Cortex Linux, enabling users to speak commands instead of typing them. The implementation uses local speech-to-text processing with faster-whisper for privacy and low latency. However, there are several critical issues that need to be addressed before merging.
Key Changes
- New voice input module with support for continuous and single-shot voice commands
- Optional
[voice]dependency group in pyproject.toml - Windows compatibility improvements in branding module (ASCII fallback for console output)
- Comprehensive documentation for voice features
- 20 unit tests for voice functionality
Reviewed changes
Copilot reviewed 12 out of 42 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_voice.py | Unit tests for voice input handler with mocked dependencies |
| requirements.txt | Issue: Voice dependencies incorrectly added as required instead of optional |
| pyproject.toml | Proper optional voice dependency group configuration |
| myenv/* | Critical: Entire virtual environment directory committed (should be excluded) |
| docs/VOICE_INPUT.md | Comprehensive user documentation for voice features |
| cortex/branding.py | Windows compatibility with ASCII fallback for special characters |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
requirements.txt (1)
9-25: Duplicate PyYAML dependencies with inconsistent versions.There are three occurrences of PyYAML in this file:
- Line 9:
PyYAML>=6.0.0- Line 21:
pyyaml>=6.0.0- Line 25:
PyYAML==6.0.3This creates confusion and potential version conflicts. Keep only one entry with a consistent version constraint.
🔎 Proposed fix
# Configuration PyYAML>=6.0.0 # Environment variable loading from .env files python-dotenv>=1.0.0 # Encryption for environment variable secrets cryptography>=42.0.0 # Terminal UI rich>=13.0.0 -# Configuration -pyyaml>=6.0.0 - # Type hints for older Python versions typing-extensions>=4.0.0 -PyYAML==6.0.3
♻️ Duplicate comments (4)
myenv/Scripts/deactivate.bat (1)
1-22: Part of virtual environment that should not be committed.This file is part of the
myenv/virtual environment directory that should be removed from version control. See the comment onmyenv/pyvenv.cfgfor details.myenv/share/man/man1/isympy.1 (1)
1-188: Third-party package artifact that should not be committed.This is a SymPy man page installed in the virtual environment. It's part of the
myenv/directory that should be removed from version control. See the comment onmyenv/pyvenv.cfgfor details.myenv/Scripts/activate.bat (1)
1-34: Part of virtual environment with hardcoded developer paths.This file contains a hardcoded path (
C:\Users\sahil\...) on line 11 and is part of themyenv/virtual environment that should be removed. See the comment onmyenv/pyvenv.cfgfor details.myenv/Scripts/Activate.ps1 (1)
1-528: Part of virtual environment that should not be committed.This PowerShell activation script is part of the
myenv/virtual environment directory that should be removed from version control. See the comment onmyenv/pyvenv.cfgfor details.
🧹 Nitpick comments (4)
docs/VOICE_INPUT.md (1)
66-83: Add language identifier to fenced code blocks for markdown lint compliance.Static analysis flagged missing language specifiers. For terminal output and diagrams, use
textorconsoleas the language identifier.🔎 Suggested fix
Line 66:
-``` +```text $ cortex voiceLine 146:
-``` +```text ┌──────────────┐ ┌──────────────┐cortex/voice.py (3)
296-307: Recording indicator bypasses branding utilities.The recording indicator at line 302 uses raw string formatting
" CX | "instead of thecx_printfunction fromcortex/branding.py. This creates inconsistency in terminal output, especially on Windows wherecx_printuses ASCII-only icons.🔎 Suggested approach
Consider using
console.printwith Rich markup for consistency, or document why direct stdout is necessary (e.g., for\rcarriage return updates).If carriage return updates are required, you could use Rich's
Livecontext orStatusfor animated updates instead of raw stdout manipulation.
425-428: Busy-wait loop could be replaced with event-based waiting.The infinite loop with
time.sleep(0.1)is a busy-wait pattern. While functional, it wastes CPU cycles polling. Consider using anEvent.wait()pattern that blocks until signaled.🔎 Alternative approach
+ self._exit_event = threading.Event() + try: # Keep the main thread alive - while True: - time.sleep(0.1) + self._exit_event.wait() except KeyboardInterrupt: cx_print("\nVoice mode exited.", "info")Then set
self._exit_event.set()in thestop()method.
482-487: Bare except clause silently swallows all exceptions.The bare
exceptat line 485 catches and ignores all exceptions, includingKeyboardInterruptandSystemExit. This could mask programming errors during debugging.🔎 Suggested fix
if self._hotkey_listener: try: self._hotkey_listener.stop() - except Exception: - pass + except OSError: + # Listener may already be stopped + pass self._hotkey_listener = NoneOr log the exception at debug level:
except Exception as e: logging.debug("Error stopping hotkey listener: %s", e)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (29)
myenv/Scripts/coloredlogs.exeis excluded by!**/*.exemyenv/Scripts/cortex.exeis excluded by!**/*.exemyenv/Scripts/ct2-fairseq-converter.exeis excluded by!**/*.exemyenv/Scripts/ct2-marian-converter.exeis excluded by!**/*.exemyenv/Scripts/ct2-openai-gpt2-converter.exeis excluded by!**/*.exemyenv/Scripts/ct2-opennmt-py-converter.exeis excluded by!**/*.exemyenv/Scripts/ct2-opennmt-tf-converter.exeis excluded by!**/*.exemyenv/Scripts/ct2-opus-mt-converter.exeis excluded by!**/*.exemyenv/Scripts/ct2-transformers-converter.exeis excluded by!**/*.exemyenv/Scripts/distro.exeis excluded by!**/*.exemyenv/Scripts/dotenv.exeis excluded by!**/*.exemyenv/Scripts/f2py.exeis excluded by!**/*.exemyenv/Scripts/hf.exeis excluded by!**/*.exemyenv/Scripts/httpx.exeis excluded by!**/*.exemyenv/Scripts/humanfriendly.exeis excluded by!**/*.exemyenv/Scripts/isympy.exeis excluded by!**/*.exemyenv/Scripts/markdown-it.exeis excluded by!**/*.exemyenv/Scripts/numpy-config.exeis excluded by!**/*.exemyenv/Scripts/onnxruntime_test.exeis excluded by!**/*.exemyenv/Scripts/openai.exeis excluded by!**/*.exemyenv/Scripts/pip.exeis excluded by!**/*.exemyenv/Scripts/pip3.12.exeis excluded by!**/*.exemyenv/Scripts/pip3.exeis excluded by!**/*.exemyenv/Scripts/pyav.exeis excluded by!**/*.exemyenv/Scripts/pygmentize.exeis excluded by!**/*.exemyenv/Scripts/python.exeis excluded by!**/*.exemyenv/Scripts/pythonw.exeis excluded by!**/*.exemyenv/Scripts/tiny-agents.exeis excluded by!**/*.exemyenv/Scripts/tqdm.exeis excluded by!**/*.exe
📒 Files selected for processing (13)
cortex/branding.pycortex/cli.pycortex/voice.pydocs/VOICE_INPUT.mdmyenv/Scripts/Activate.ps1myenv/Scripts/activatemyenv/Scripts/activate.batmyenv/Scripts/deactivate.batmyenv/pyvenv.cfgmyenv/share/man/man1/isympy.1pyproject.tomlrequirements.txttests/test_voice.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/test_voice.pycortex/voice.pycortex/branding.pycortex/cli.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_voice.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:
pyproject.toml
🧬 Code graph analysis (3)
tests/test_voice.py (1)
cortex/voice.py (9)
VoiceInputHandler(39-495)_ensure_dependencies(87-125)_check_microphone(153-175)transcribe(226-270)stop(477-495)VoiceInputError(21-24)MicrophoneNotFoundError(27-30)ModelNotFoundError(33-36)get_voice_handler(498-517)
cortex/voice.py (1)
cortex/branding.py (1)
cx_print(52-82)
cortex/cli.py (1)
cortex/voice.py (4)
VoiceInputError(21-24)VoiceInputHandler(39-495)start_voice_mode(399-432)record_single(434-475)
🪛 GitHub Actions: CI
cortex/cli.py
[error] 580-580: Ruff check failed: W293 Blank line contains whitespace. Command: 'ruff check . --output-format=github'.
🪛 GitHub Check: lint
cortex/cli.py
[failure] 591-591: Ruff (W293)
cortex/cli.py:591:1: W293 Blank line contains whitespace
[failure] 588-588: Ruff (W293)
cortex/cli.py:588:1: W293 Blank line contains whitespace
[failure] 580-580: Ruff (W293)
cortex/cli.py:580:1: W293 Blank line contains whitespace
🪛 GitHub Check: Lint
cortex/cli.py
[failure] 591-591: Ruff (W293)
cortex/cli.py:591:1: W293 Blank line contains whitespace
[failure] 588-588: Ruff (W293)
cortex/cli.py:588:1: W293 Blank line contains whitespace
[failure] 580-580: Ruff (W293)
cortex/cli.py:580:1: W293 Blank line contains whitespace
🪛 LanguageTool
docs/VOICE_INPUT.md
[grammar] ~141-~141: Ensure spelling is correct
Context: ...apture** - Records via sounddevice at 16kHz mono 3. Speech-to-Text - Transcribe...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~141-~141: Ensure spelling is correct
Context: ... Records via sounddevice at 16kHz mono 3. Speech-to-Text - Transcribes using `fa...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
docs/VOICE_INPUT.md
66-66: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
146-146: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Agent
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
- GitHub Check: test (3.10)
🔇 Additional comments (14)
cortex/branding.py (3)
8-14: Good Windows compatibility improvements for Rich console.The platform-aware Console initialization with
force_terminal=Trueandlegacy_windowsbased on platform detection is a solid approach for cross-platform terminal rendering.
63-79: Well-structured platform-specific icon fallbacks.The ASCII fallback icons for Windows (
|,+,!,x,*) are appropriate replacements for the Unicode characters that may not render correctly in Windows terminals. The conditional structure is clean and maintainable.
92-102: Consistent platform-specific separators.The separator adjustments in
cx_stepandcx_headeralign with the icon changes above, ensuring consistent visual appearance across platforms.pyproject.toml (1)
72-79: Voice optional dependencies are correctly structured.The optional dependency group is properly defined with appropriate version constraints compatible with Python 3.10+. Including it in the
allextra is appropriate for comprehensive installation.docs/VOICE_INPUT.md (1)
1-46: Well-structured documentation with comprehensive coverage.The documentation covers all essential aspects: installation, usage modes (continuous and single), configuration options, troubleshooting, privacy considerations, and API reference. The privacy-first approach (local processing, no audio uploads) is well highlighted.
tests/test_voice.py (1)
10-11: Good test organization with clear class-based grouping.The test suite is well-organized into logical test classes (
TestVoiceInputHandler,TestVoiceInputExceptions,TestGetVoiceHandler,TestRecordingState), each with focused test methods and appropriate fixtures. The mocking strategy for optional dependencies is sound.Also applies to: 199-200, 224-225, 268-269
cortex/cli.py (3)
540-552: Good implementation with proper dependency handling.The voice method correctly handles the optional dependency import with a helpful error message guiding users to install the voice extras. The API key and provider checks follow the established pattern in the codebase.
560-577: Consider case-insensitive verb matching and edge cases.The install verb detection uses
startswith()after lowercasing, which is good. However, the verb removal at lines 574-577 operates on the originaltextbut useslen(verb)from the lowercase version, which works correctly since length is preserved. The logic is sound.One edge case: if the user says "Install", the slicing
text[len(verb):]correctly preserves the original casing of the software name.
1633-1638: Help table correctly advertises new voice capabilities.The Rich help table now includes voice command entries, making the feature discoverable to users.
cortex/voice.py (5)
21-36: Well-designed exception hierarchy.The exception classes are properly structured with a base
VoiceInputErrorand specific subclasses for different error conditions. Docstrings are present as required by coding guidelines. Thepassstatements are appropriate for simple exception classes.
51-85: Good initialization with sensible defaults and type hints.The
__init__method has proper type hints, comprehensive docstrings, and sensible defaults. The environment variable fallback formodel_nameprovides flexibility. Internal state management with explicit initialization is clean.
226-270: Transcription method is well-implemented with proper validation.The
transcribemethod correctly:
- Handles lazy model loading (line 238-239)
- Returns early for empty audio (line 241-242)
- Validates dtype before processing (line 245-246)
- Uses appropriate VAD parameters for voice commands
- Strips and joins segment texts properly
498-517: Factory function provides clean API.The
get_voice_handlerfactory function has proper type hints and docstring. It provides a simpler interface for common use cases while still allowing customization.
177-202: Audio recording implementation is robust.The
_start_recordingmethod properly:
- Clears the buffer before starting
- Uses a callback-based approach for non-blocking capture
- Sets up appropriate stream parameters (16kHz, mono, float32)
- Handles errors by resetting state and raising a specific exception
|
@Sahilbhatane remove all those virtual environment files. You should never push those. |
yeah ik, .gitignore didn't recognized them ig. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
cortex/voice.py (1)
486-490: Improve exception handling in hotkey listener cleanup.The bare
except Exception: passsilently swallows all errors. Following the pattern used at line 498 for the audio stream, add debug logging to track potential issues during cleanup.🔎 Proposed fix
if self._hotkey_listener: try: self._hotkey_listener.stop() - except Exception: - pass + except Exception as e: + logging.debug("Error stopping hotkey listener: %s", e) self._hotkey_listener = Nonecortex/cli.py (3)
554-558: Unusedprovidervariable.The
providervariable is retrieved at line 558 but never used in thevoicemethod. Theprocess_voice_commandcallback already retrieves the API key and usesself.ask()andself.install(), which internally get the provider themselves.🔎 Consider removing unused variable
api_key = self._get_api_key() if not api_key: return 1 - - provider = self._get_provider()
579-600: Fix trailing whitespace to resolve pipeline failures.Based on past review comments, lines 580, 588, and 591 contain trailing whitespace on blank lines, causing CI/linter failures. These must be fixed for the PR to pass.
🔎 Remove trailing whitespace
Ensure lines 580, 588, and 591 are completely empty (no spaces or tabs):
cx_print(f"Installing: {software}", "info") - + # Ask user for confirmation console.print() console.print("[bold cyan]Choose an action:[/bold cyan]") console.print(" [1] Dry run (preview commands)") console.print(" [2] Execute (run commands)") console.print(" [3] Cancel") console.print() - + try: choice = input("Enter choice [1/2/3]: ").strip() - + if choice == "1":
2000-2015: Add error handling for VoiceInputError and its subclasses.The
install --micflow catchesImportErrorbut doesn't handleVoiceInputError,MicrophoneNotFoundError, orModelNotFoundError. If voice recording fails (e.g., no microphone detected, model loading fails), an unhandled exception will propagate.🔎 Proposed fix
if getattr(args, "mic", False): try: - from cortex.voice import VoiceInputHandler + from cortex.voice import VoiceInputError, VoiceInputHandler handler = VoiceInputHandler() cx_print("Press F9 to speak what you want to install...", "info") software = handler.record_single() if not software: cx_print("No speech detected.", "warning") return 1 cx_print(f"Installing: {software}", "info") except ImportError: cli._print_error("Voice dependencies not installed.") cx_print("Install with: pip install cortex-linux[voice]", "info") return 1 + except VoiceInputError as e: + cli._print_error(str(e)) + return 1
🧹 Nitpick comments (1)
.gitignore (1)
14-20: Consolidate duplicate entries for maintainability.The
.gitignorefile contains numerous redundant entries. For example:
env/,venv/,ENV/appear at lines 14–15, 143–145.mypy_cache/,.pytest_cache/,.coverage,htmlcov/appear at lines 70–71 and 146–149This duplication reduces readability and makes future maintenance harder.
🔎 Suggested cleanup: Remove duplicates and consolidate sections
# ============================== # Logs & Misc # ============================== *.log logs/ *.tmp *.bak *.swp - .env - .venv - env/ - venv/ - ENV/ - .mypy_cache/ - .pytest_cache/ - .coverage - htmlcov/ *.out *~ *.swoThen, verify that all non-duplicate entries are already covered in their respective sections above (Virtual Environments, mypy/Pyre/pytype, etc.).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (23)
myenv/Scripts/bandit-baseline.exeis excluded by!**/*.exemyenv/Scripts/bandit-config-generator.exeis excluded by!**/*.exemyenv/Scripts/bandit.exeis excluded by!**/*.exemyenv/Scripts/black.exeis excluded by!**/*.exemyenv/Scripts/blackd.exeis excluded by!**/*.exemyenv/Scripts/coverage-3.12.exeis excluded by!**/*.exemyenv/Scripts/coverage.exeis excluded by!**/*.exemyenv/Scripts/coverage3.exeis excluded by!**/*.exemyenv/Scripts/dmypy.exeis excluded by!**/*.exemyenv/Scripts/mypy.exeis excluded by!**/*.exemyenv/Scripts/mypyc.exeis excluded by!**/*.exemyenv/Scripts/nltk.exeis excluded by!**/*.exemyenv/Scripts/normalizer.exeis excluded by!**/*.exemyenv/Scripts/pip.exeis excluded by!**/*.exemyenv/Scripts/pip3.12.exeis excluded by!**/*.exemyenv/Scripts/pip3.exeis excluded by!**/*.exemyenv/Scripts/py.test.exeis excluded by!**/*.exemyenv/Scripts/pytest.exeis excluded by!**/*.exemyenv/Scripts/ruff.exeis excluded by!**/*.exemyenv/Scripts/safety.exeis excluded by!**/*.exemyenv/Scripts/stubgen.exeis excluded by!**/*.exemyenv/Scripts/stubtest.exeis excluded by!**/*.exemyenv/Scripts/typer.exeis excluded by!**/*.exe
📒 Files selected for processing (6)
.gitignorecortex/cli.pycortex/voice.pymyenv/share/man/man1/bandit.1tests/test_ollama_integration.pytests/test_voice.py
✅ Files skipped from review due to trivial changes (1)
- myenv/share/man/man1/bandit.1
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_voice.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs
Files:
tests/test_ollama_integration.pycortex/voice.pycortex/cli.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_ollama_integration.py
🧬 Code graph analysis (3)
tests/test_ollama_integration.py (1)
scripts/setup_ollama.py (1)
check_ollama_installed(75-77)
cortex/voice.py (1)
cortex/branding.py (1)
cx_print(52-82)
cortex/cli.py (1)
cortex/voice.py (4)
VoiceInputError(21-24)VoiceInputHandler(39-499)start_voice_mode(401-436)record_single(438-479)
⏰ 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). (6)
- GitHub Check: Test (Python 3.12)
- GitHub Check: Test (Python 3.10)
- GitHub Check: Test (Python 3.11)
- GitHub Check: test (3.12)
- GitHub Check: test (3.10)
- GitHub Check: test (3.11)
🔇 Additional comments (10)
tests/test_ollama_integration.py (1)
15-15: LGTM: Clean refactor toshutil.which.Replacing the subprocess-based Ollama detection with
shutil.whichis a cleaner, more direct approach that aligns with the pattern used inscripts/setup_ollama.py(lines 74-76).Also applies to: 29-30, 37-37
cortex/voice.py (5)
1-36: LGTM: Clean module structure and exception hierarchy.Module docstring clearly describes the feature, and the exception hierarchy provides appropriate granularity for error handling.
39-86: LGTM: Well-structured initialization with environment-driven configuration.Good use of lazy loading for the model and environment variable overrides. The type hints and docstring meet coding guidelines.
87-270: LGTM: Robust dependency checks and transcription pipeline.The dependency validation provides helpful user guidance, and the transcription implementation uses appropriate parameters (beam_size=5, VAD filtering, no_speech_threshold=0.6).
272-436: LGTM: Thread-safe hotkey handling with good UX.The implementation properly uses threading primitives (locks, events) and provides visual feedback during recording. KeyboardInterrupt handling ensures clean exit.
502-521: LGTM: Clean factory function with proper type hints.The factory function follows Python conventions and provides a clean entry point for creating voice handlers.
cortex/cli.py (4)
1633-1633: LGTM: Help documentation clearly advertises voice features.The help table updates appropriately document the new voice input capabilities for users.
Also applies to: 1638-1638
1722-1739: LGTM: Well-structured argument parsing for voice features.The
--micand--singleflags are appropriately configured, and making the question argument optional enables the voice input flow.
1743-1755: LGTM: Consistent --mic flag integration.The install command's
--micflag follows the same pattern as the ask command, maintaining consistency across the CLI.
1989-1998: LGTM: Clean command routing for voice features.The routing logic appropriately delegates to the voice method for both explicit voice commands and
--micflags, with helpful error messages.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
cortex/voice.py (2)
185-189: Consider using logging for audio status messages.Line 187 prints audio status directly to stderr. For consistency with the rest of the module (which uses
cx_printandlogging.debug), consider usinglogging.debug()instead.🔎 Proposed refactor
def audio_callback(indata, frames, time_info, status): if status: - print(f"Audio status: {status}", file=sys.stderr) + logging.debug("Audio status: %s", status) if self._is_recording: self._audio_buffer.append(indata.copy())
238-250: Redundant model check after lazy loading.Lines 248-250 check if
self._model is Noneafter calling_load_model()on line 239. However,_load_model()either setsself._modelor raisesModelNotFoundError, so the model can never beNoneat line 249. This check is dead code.🔎 Proposed refactor
if self._model is None: self._load_model() if len(audio_data) == 0: return "" # faster-whisper expects float32 audio normalized to [-1, 1] if audio_data.dtype != np.float32: audio_data = audio_data.astype(np.float32) - # Model should be loaded at this point - if self._model is None: - raise ModelNotFoundError("Model must be loaded before transcription") - segments, info = self._model.transcribe(docs/VOICE_INPUT.md (1)
66-66: Consider adding language identifiers to code blocks.The fenced code blocks at lines 66 and 146 are missing language identifiers. While not critical, adding them improves syntax highlighting and documentation quality.
🔎 Suggested additions
For line 66 (example session output):
-``` +```console $ cortex voiceFor line 146 (ASCII diagram):
-``` +```text ┌──────────────┐ ┌──────────────┐ ┌──────────────┐Also applies to: 146-157
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cortex/cli.pycortex/voice.pydocs/VOICE_INPUT.mdrequirements.txttests/test_voice.py
🚧 Files skipped from review as they are similar to previous changes (2)
- requirements.txt
- tests/test_voice.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/cli.pycortex/voice.py
🧬 Code graph analysis (2)
cortex/cli.py (3)
cortex/voice.py (4)
VoiceInputError(21-24)VoiceInputHandler(39-499)start_voice_mode(401-436)record_single(438-479)cortex/first_run_wizard.py (1)
_print_error(746-748)cortex/branding.py (1)
cx_print(52-82)
cortex/voice.py (1)
cortex/branding.py (1)
cx_print(52-82)
🪛 LanguageTool
docs/VOICE_INPUT.md
[grammar] ~141-~141: Ensure spelling is correct
Context: ...apture** - Records via sounddevice at 16kHz mono 3. Speech-to-Text - Transcribe...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~141-~141: Ensure spelling is correct
Context: ... Records via sounddevice at 16kHz mono 3. Speech-to-Text - Transcribes using `fa...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
docs/VOICE_INPUT.md
66-66: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
146-146: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (10)
cortex/voice.py (3)
339-383: LGTM! Thread-safe hotkey handling.The hotkey setup properly uses a lock for thread safety and correctly sets the recording flag before starting threads. The logic handles both press-to-start and press-to-stop flows correctly.
385-436: LGTM! Proper error handling and resource cleanup.The recording worker and continuous voice mode implementation correctly handle errors and ensure resources are cleaned up via the
finallyblock.
481-499: LGTM! Exception handling improved.The cleanup logic properly handles exceptions during shutdown and logs them for debugging. This addresses the past review comment about silent exception handling.
docs/VOICE_INPUT.md (2)
49-49: LGTM! Model size documentation is now consistent.The documentation correctly states that the default
base.enmodel is ~150MB, which is consistent with the table at line 117. This addresses the past review comment about the documentation inconsistency.
1-261: LGTM! Comprehensive and well-structured documentation.The documentation provides excellent coverage of the voice input feature, including installation, usage, configuration, troubleshooting, and API reference. The structure is clear and user-friendly.
cortex/cli.py (5)
540-625: LGTM! Well-designed voice input integration.The
voice()method properly handles both continuous and single-shot modes, includes user confirmation for installations, and has comprehensive error handling for missing dependencies and voice input errors.
1989-1996: LGTM! Clean integration of voice input with ask command.The
--micflag integration properly routes to the voice handler and provides clear error messages when neither a question nor the mic flag is provided.
1998-2027: LGTM! VoiceInputError handling properly implemented.The install command's
--micintegration now correctly imports and catchesVoiceInputError(lines 2001, 2014-2016), addressing the past review comment about missing error handling. The implementation provides clear error messages and proper fallback behavior.
1728-1753: LGTM! Parser configuration is correct.The new voice command and
--micflag integrations are properly configured with clear help text and appropriate argument handling. Makingsoftwareoptional (line 1741) correctly supports the--micworkflow.
1987-1988: LGTM! Voice command routing is correct.The routing logic correctly maps the
--singleflag to thecontinuousparameter by negating it, so that by default (no flag) continuous mode is active, and with--singleit switches to single-shot mode.
Sahilbhatane
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @cortex/cli.py:
- Around line 2656-2674: VoiceInputHandler is instantiated in the --mic install
path but handler.stop() isn't guaranteed to run if an exception occurs; wrap
creation and usage of VoiceInputHandler (the handler = VoiceInputHandler();
software = handler.record_single(); cx_print(...) flow) in a try/finally (or use
a context manager if available) and call handler.stop() in the finally block so
resources are always cleaned up even on exceptions (ensure you still catch
ImportError and VoiceInputError as before and only return after stop() is
called).
In @requirements.txt:
- Around line 22-24: Remove the duplicated dependency and stray comment by
deleting the repeated "# Configuration" comment and the duplicate
"PyYAML>=6.0.0" entry (the second occurrence) so only the original comment and
single "PyYAML>=6.0.0" remain in requirements.txt; ensure no other duplicate
package lines exist to avoid being listed twice by setup.py.
🧹 Nitpick comments (3)
cortex/voice.py (2)
347-360: Consider terminal width for line clearing.The hardcoded 70 spaces (line 360) may not fully clear the line on wider terminals or may wrap on narrower ones. This is a minor cosmetic issue.
♻️ Optional improvement
# Clear the line - console.print(" " * 70, end="\r") + import shutil + cols = shutil.get_terminal_size().columns + console.print(" " * min(cols - 1, 70), end="\r")
362-388: Consider adding return type hint.The
_get_hotkey_keymethod could benefit from a return type annotation for clarity, though it's a private method.♻️ Optional improvement
- def _get_hotkey_key(self): + def _get_hotkey_key(self) -> Any: """Get the pynput key object for the configured hotkey."""Or more precisely with the actual type when pynput is available.
tests/test_cli_extended.py (1)
43-52: LGTM! The addedPath.cwdpatch aligns with Ollama local key resolution.The change correctly patches both
Path.homeandPath.cwdfor this specific test case where environment variables are cleared and detection falls through to interactive mode. Other tests (test_get_api_key_openai,test_get_api_key_claude) don't need thePath.cwdpatch since they set environment variables that short-circuit before reaching that code path.Optional: Consider using
contextlib.ExitStackor combining patches to reduce nesting depth (4 levels here), though this is consistent with the file's existing style.♻️ Optional: Flatten nested context managers
def test_get_api_key_not_found(self) -> None: # When no API key is set and user selects Ollama, falls back to Ollama local mode from cortex.api_key_detector import PROVIDER_MENU_CHOICES - with patch.dict(os.environ, {}, clear=True): - with patch("pathlib.Path.home", return_value=self._temp_home): - with patch("pathlib.Path.cwd", return_value=self._temp_home): - with patch("builtins.input", return_value=PROVIDER_MENU_CHOICES["ollama"]): - api_key = self.cli._get_api_key() - self.assertEqual(api_key, "ollama-local") + with ( + patch.dict(os.environ, {}, clear=True), + patch("pathlib.Path.home", return_value=self._temp_home), + patch("pathlib.Path.cwd", return_value=self._temp_home), + patch("builtins.input", return_value=PROVIDER_MENU_CHOICES["ollama"]), + ): + api_key = self.cli._get_api_key() + self.assertEqual(api_key, "ollama-local")
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.gitignorecortex/cli.pycortex/voice.pyrequirements.txtsetup.pytests/integration/test_end_to_end.pytests/test_api_key_detector.pytests/test_cli.pytests/test_cli_extended.pytests/test_ollama_integration.pytests/test_voice.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/test_voice.py
- .gitignore
- tests/test_ollama_integration.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/test_api_key_detector.pytests/test_cli_extended.pycortex/cli.pytests/test_cli.pysetup.pytests/integration/test_end_to_end.pycortex/voice.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_api_key_detector.pytests/test_cli_extended.pytests/test_cli.pytests/integration/test_end_to_end.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:
setup.py
🧬 Code graph analysis (4)
tests/test_api_key_detector.py (1)
cortex/api_key_detector.py (1)
detect(77-123)
tests/test_cli_extended.py (1)
cortex/cli.py (1)
_get_api_key(130-152)
tests/test_cli.py (1)
cortex/cli.py (1)
_get_api_key(130-152)
cortex/voice.py (1)
cortex/branding.py (1)
cx_print(52-82)
🔇 Additional comments (14)
requirements.txt (1)
26-40: Informational comment block looks good.The documentation for the optional voice feature dependencies and system requirements is helpful for developers who want to enable voice input.
tests/integration/test_end_to_end.py (1)
20-22: LGTM!The
PIP_BOOTSTRAP_DEVconstant is well-defined and appropriately used intest_project_tests_run_inside_containerto install dev dependencies needed for pytest.cortex/cli.py (3)
637-743: Voice method implementation is well-structured.The
voice()method properly handles:
- Dependency import failures with helpful error messages
- API key validation
- Continuous vs single-shot modes
- Input validation for install commands (length, dangerous characters)
- Resource cleanup in the
finallyblock
744-901: Refactored install helpers improve maintainability.The extraction of
_handle_parallel_execution,_handle_sequential_execution, and related utilities from theinstall()method reduces complexity and improves readability. Error handling is consistent throughout.
2253-2286: Voice CLI integration is well-designed.The
voicesubcommand with--singleflag and--micflags onask/installprovide flexible voice input options. Help text clearly describes the functionality.cortex/voice.py (6)
19-34: Exception hierarchy is well-designed.The exception classes provide clear, specific error types for voice input failures with appropriate inheritance from
VoiceInputError.
49-83: Initialization is thorough with sensible defaults.Environment variable support for
CORTEX_WHISPER_MODELprovides flexibility. Threading primitives are properly initialized.
206-273: Recording implementation handles resources correctly.Good practices observed:
- Memory protection with ~60 second buffer limit (line 221)
- Proper stream cleanup in
_stop_recording_stream- Setting
_is_recording = Falsebefore stopping stream prevents race conditions- Buffer cleared in
finallyblock to prevent memory leaks
275-321: Transcription method is well-configured.The VAD (Voice Activity Detection) parameters and
condition_on_previous_text=Falsesetting are appropriate for command-style speech. Lazy model loading improves startup time.
390-434: Hotkey handling uses proper synchronization.The
recording_lockcorrectly serializes access to the recording state, preventing race conditions when the hotkey is pressed rapidly. The daemon threads ensure cleanup on program exit.
532-550: Cleanup method is robust.The
stop()method properly handles multiple cleanup scenarios with appropriate exception handling, preventing errors from leaking if resources are already cleaned up.tests/test_cli.py (1)
46-49: Test isolation improvement is correct.Adding the
pathlib.Path.cwdpatch ensures the test doesn't accidentally find.envfiles in the actual current working directory, improving test reliability and isolation.tests/test_api_key_detector.py (1)
162-166: Test isolation fix is consistent with other test changes.Patching
pathlib.Path.cwdalongsidePath.homeensures complete isolation from the real filesystem, preventing false positives if the test runner's directory contains.envfiles.setup.py (1)
48-55: Voice extras definition is correct and properly synced across configuration files.The optional
voicedependency group is properly structured with appropriate version constraints in both setup.py and pyproject.toml, and is correctly included in theallextra for comprehensive dependency installation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
cortex/cli.py (4)
679-683: Consider expanding dangerous character validation.The current list covers common shell metacharacters but misses some injection vectors like newlines (
\n,\r), backslashes (\), and quotes (',"). For defense-in-depth, consider using an allowlist pattern instead.♻️ Suggested improvement
- # Check for dangerous characters that shouldn't be in package names - dangerous_chars = [";", "&", "|", "`", "$", "(", ")"] - if any(char in software for char in dangerous_chars): - cx_print("Invalid characters detected in software name", "error") - return + # Check for dangerous characters that shouldn't be in package names + dangerous_chars = [";", "&", "|", "`", "$", "(", ")", "\n", "\r", "\\", "'", '"'] + if any(char in software for char in dangerous_chars): + cx_print("Invalid characters detected in software name", "error") + return
695-706: Consider usingconsole.inputfor consistent styling.The prompt uses
input()while other parts of the CLI useconsole.input()from Rich for styled prompts. This inconsistency may result in visual differences.♻️ Optional: Use Rich console for consistent styling
try: - choice = input("Enter choice [1/2/3]: ").strip() + choice = console.input("[bold cyan]Enter choice [1/2/3]: [/bold cyan]").strip()
744-753: Hardcoded special case is a code smell.This normalization logic embeds a specific package combination directly in code. Consider moving such mappings to a configuration file or the existing
stacks.jsonfor maintainability.
846-849: Fragile status comparison using string value.Using
getattr(t.status, "value", "") == "failed"is fragile. Ift.statusis an enum, compare directly against the enum member for type safety.♻️ Suggested improvement
def _get_parallel_error_msg(self, parallel_tasks: list) -> str: """Extract error message from failed parallel tasks.""" - failed_tasks = [t for t in parallel_tasks if getattr(t.status, "value", "") == "failed"] + from cortex.coordinator import StepStatus + failed_tasks = [t for t in parallel_tasks if t.status == StepStatus.FAILED] return failed_tasks[0].error if failed_tasks else self.INSTALL_FAIL_MSG
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cortex/cli.pyrequirements.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- requirements.txt
🧰 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/cli.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Test (Python 3.10)
- GitHub Check: Test (Python 3.12)
- GitHub Check: Test (Python 3.11)
- GitHub Check: test (3.11)
- GitHub Check: test (3.10)
- GitHub Check: test (3.12)
🔇 Additional comments (11)
cortex/cli.py (11)
10-10: LGTM!Import is correctly placed with other cortex module imports.
39-41: LGTM!Good extraction of a repeated string literal into a class constant for maintainability.
154-175: LGTM!Comment clarifications improve code readability without changing functionality.
712-742: LGTM!Proper resource cleanup pattern with
handler = Noneinitialization before try block and cleanup in finally. The nested try/except in the finally block correctly prevents cleanup errors from masking the original exception.
765-827: LGTM!Good error handling with specific exception types (ValueError, OSError) and a catch-all for unexpected errors. The parallel execution logic correctly propagates errors to history.
851-901: LGTM!Sequential execution handler is well-structured with proper progress callbacks and comprehensive error recording.
903-978: LGTM!Good refactoring of the install method. The extraction of parallel and sequential execution into helper methods improves readability and maintainability. Return type annotation adds clarity.
2146-2152: LGTM!Help table appropriately documents the new voice input features with clear descriptions.
2253-2270: LGTM!Voice command parser is well-designed with
--singleflag providing flexibility between continuous and single-shot modes. The optional question argument properly supports the--micflag workflow.
2656-2694: LGTM!The
--micflag handling for install is correctly structured:
- Proper try/finally for resource cleanup
- Early return on voice detection failure
- Variable
softwareis correctly set in both branches before use at line 2689
2078-2078: LGTM!Consistent use of the
INSTALL_FAIL_MSGclass constant across error handlers.
base.en model doesn't recognize the speech properly |
|
Added selection mode for user where they can choose the model they want for there system, also stating clearly small model = low accuracy during speech to text |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@cortex/branding.py`:
- Around line 14-22: The Console is being initialized with force_terminal=True
and legacy_windows=sys.platform == "win32", which overrides Rich's
auto-detection; update the Console creation in branding.py (the Console
constructor assigned to console) to either remove both parameters so Rich can
auto-detect, or if you truly need forced styling keep only force_terminal=True
and drop the legacy_windows platform check so you don't enable legacy mode on
all Windows systems—modify the Console(...) call accordingly and ensure any
downstream code using the console symbol continues to work with the new
initialization.
In `@cortex/voice.py`:
- Around line 444-458: The worker _recording_worker can leave callers blocked
because record_single waits on done_event but the hotkey callback
(_hotkey_callback) is only invoked when text is non-empty; modify
_recording_worker to always signal completion to the caller: ensure that
regardless of whether text is non-empty, empty, or an exception occurs you
invoke the done callback (or call _hotkey_callback with an empty string/explicit
completion token) or otherwise set/trigger the same completion mechanism used by
record_single (referencing record_single, done_event, _hotkey_callback,
record_and_transcribe, and _is_recording) so the waiting thread is unblocked;
keep existing logging behavior for empty transcription and exceptions but
guarantee the completion callback/path is executed in all branches (including
except and finally).
In `@docs/VOICE_INPUT.md`:
- Around line 65-83: The fenced code blocks in VOICE_INPUT.md example session
and the ASCII diagram (around the blocks shown) are missing a language specifier
which triggers MD040; update both triple-backtick fences to include a language
(use "text") so they read ```text at the start of each block (the example
session block and the ASCII diagram block referenced in the diff) to satisfy
markdownlint.
In `@pyproject.toml`:
- Around line 90-98: Update the pynput dependency in the voice extras from
"pynput>=1.7.6" to "pynput>=1.8.1" to ensure compatibility with Python 3.10+,
and add a note in the project installation/setup docs (or README) stating that
sounddevice requires the system PortAudio library (libportaudio2 on Linux) to be
installed before pip installing the dependencies; adjust the "voice" extras
declaration and the docs/installation section accordingly.
♻️ Duplicate comments (4)
cortex/api_key_detector.py (1)
160-176: Explicit provider in encrypted storage still skipsanthropic.
The explicit-provider branch doesn’t include"anthropic", soCORTEX_PROVIDER=anthropicwon’t be honored before the default order.🔧 Suggested fix
- if explicit_provider in ["openai", "claude"]: + if explicit_provider in ["openai", "claude", "anthropic"]:tests/integration/test_end_to_end.py (1)
20-23: Add python3-dev and kernel-matched headers for evdev builds.
pynput/evdevcompilation typically requires Python headers, andlinux-headers-genericmay not match the running kernel in the container. This can cause build failures during integration tests.🔧 Proposed fix
-SYSTEM_DEPS_INSTALL = "apt-get update && apt-get install -y --no-install-recommends linux-headers-generic build-essential libasound2-dev libportaudio2 portaudio19-dev" +SYSTEM_DEPS_INSTALL = "apt-get update && apt-get install -y --no-install-recommends linux-headers-$(uname -r) build-essential python3-dev libasound2-dev libportaudio2 portaudio19-dev"cortex/cli.py (2)
889-909: Avoidinput()from the recording worker thread.
process_voice_command()is invoked from the recording worker thread, but it callsinput()(Line 897). Blocking stdin on a background thread can deadlock or corrupt terminal I/O on some platforms. Consider queueing the prompt to the main thread (or a UI loop) and handling the choice there.
838-913: Prevent repeated API key prompts in continuous voice sessions.
voice()obtains an API key once (Line 838), butprocess_voice_command()callsself.install()/self.ask()which fetch keys again. In continuous mode this can re-prompt unexpectedly. Consider passing the session key/provider into internal helpers to avoid re-detection.
🧹 Nitpick comments (2)
cortex/voice.py (1)
370-396: Add a return type for_get_hotkey_key.Type hints are required, and this method currently has none. Consider
-> Any | None(or a forward reference to the pynput Key type) to keep annotations consistent. As per coding guidelines, type hints are required.cortex/cli.py (1)
966-1103: Remove unreachable install block or wire helpers into the main flow.The new helper methods are never called because the inline execution path returns earlier, leaving the block at Lines 1351-1355 unreachable. This is dead code and risks drift between duplicate implementations. Consider removing the unreachable block or refactoring
install()to route through the helpers.✂️ Minimal cleanup (remove dead block)
- print("\nExecuting commands...") - if parallel: - return self._handle_parallel_execution(commands, software, install_id, history) - - return self._handle_sequential_execution(commands, software, install_id, history)Also applies to: 1347-1355
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@cortex/cli.py`:
- Around line 983-1087: process_voice_command blocks waiting on response_queue
for install prompts even when continuous is False because input_handler_loop is
only started in continuous mode; fix by handling prompts inline for single-shot
mode: detect install in process_voice_command and when continuous is False (or
when input_handler_thread isn't started) directly prompt the user (reuse the
prompt logic from input_handler_loop: print choices, read input(), and put the
choice into response_queue or act on it immediately) instead of waiting for a
worker-to-main-thread queue roundtrip; alternatively start input_handler_thread
for single-shot before calling handler.record_single(), referencing
process_voice_command, input_handler_loop, input_queue, response_queue,
continuous, and handler.record_single to locate where to implement the inline
prompt or thread-start fix.
In `@cortex/voice.py`:
- Around line 85-213: In _ensure_dependencies replace the final "if missing: ...
return False" behavior with raising a VoiceInputError that includes the list of
missing packages (you can still cx_print the message before raising if desired);
ensure the exception message lists missing and suggested pip install commands.
In _check_microphone change the "if not input_devices: cx_print(...); return
False" to raise MicrophoneNotFoundError with the same message, and on the
general except block re-raise a VoiceInputError (raise VoiceInputError(f"Error
checking microphone: {e}") from e) instead of returning False so callers like
start_voice_mode() and record_single() receive proper exceptions.
In `@tests/integration/test_end_to_end.py`:
- Around line 20-23: The apt package string constant SYSTEM_DEPS_INSTALL
currently uses linux-headers-$(uname -r) which fails in the python:3.11-slim
container; update SYSTEM_DEPS_INSTALL to replace linux-headers-$(uname -r) with
linux-libc-dev so that PIP_BOOTSTRAP and PIP_BOOTSTRAP_DEV (which use that
constant) install the correct headers for building evdev/pynput.
🧹 Nitpick comments (1)
cortex/cli.py (1)
1511-1519: Remove unreachable execution block after the early return.
Lines 1515–1519 are dead code because the method already returns in theexecute/elsebranches. Either remove them or move the helper calls into theexecutepath.♻️ Suggested cleanup
- print("\nExecuting commands...") - if parallel: - return self._handle_parallel_execution(commands, software, install_id, history) - - return self._handle_sequential_execution(commands, software, install_id, history)
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@cortex/cli.py`:
- Around line 1016-1028: The prompt handling currently treats blank input as
cancel; update both prompt branches that set the variable choice (the branch
guarded by input_handler_thread is None and the alternate interactive branch) so
that after reading input from input("Enter choice [1/2/3]: ").strip() an empty
string is interpreted as "1" (dry-run) instead of "3"; ensure the except
(KeyboardInterrupt, EOFError) behavior remains unchanged and that any other
reads that assign to choice follow the same default-empty -> "1" rule (look for
the input_handler_thread, choice variable and the other prompt block
referenced).
In `@cortex/voice.py`:
- Around line 49-69: The constructor (__init__) currently ignores the
CORTEX_VOICE_HOTKEY environment variable; update it so hotkey is set to the
provided hotkey if truthy, otherwise fall back to
os.environ.get("CORTEX_VOICE_HOTKEY", "f9") and then lower() the value before
assigning to self.hotkey; also ensure the same default and env-var precedence is
mirrored in get_voice_handler so both places behave consistently when no
explicit hotkey is passed.
♻️ Duplicate comments (3)
cortex/voice.py (1)
462-529: Surface dependency/microphone errors to callers instead of returning success.
start_voice_mode()/record_single()catch and return on setup failures, which causes the CLI to exit 0 and contradicts the “Raises” docstrings. Re-raise after logging (or remove the try/except) so callers can fail fast.🛠️ Suggested fix
try: self._ensure_dependencies() self._check_microphone() except (VoiceInputError, MicrophoneNotFoundError) as e: cx_print(str(e), "error") - return + raise @@ try: self._ensure_dependencies() self._check_microphone() except (VoiceInputError, MicrophoneNotFoundError) as e: cx_print(str(e), "error") - return "" + raisetests/test_voice.py (2)
72-84:test_init_hotkey_from_env_vardoesn’t exercise the env var path.Either set
CORTEX_VOICE_HOTKEYand assert the result, or rename the test to reflect constructor-only behavior.✅ Suggested fix (once env var is supported)
# Test default hotkey handler = VoiceInputHandler() assert handler.hotkey == "f9" + + # Test hotkey from env var + monkeypatch.setenv("CORTEX_VOICE_HOTKEY", "f10") + handler = VoiceInputHandler() + assert handler.hotkey == "f10"
100-107:test_ensure_dependencies_missingbypasses the real failure path.Mocking
_ensure_dependencies()itself only validates the mock. The production behavior is to raiseVoiceInputError; the test should assert that.🧪 Suggested fix
- # Test that ensure_dependencies returns False when import fails - with patch.object(handler, "_ensure_dependencies") as mock_deps: - mock_deps.return_value = False - result = handler._ensure_dependencies() - assert result is False + with patch.dict( + "sys.modules", + { + "sounddevice": None, + "faster_whisper": MagicMock(), + "pynput": MagicMock(), + "pynput.keyboard": MagicMock(), + }, + clear=False, + ): + with pytest.raises(VoiceInputError): + handler._ensure_dependencies()
🧹 Nitpick comments (4)
tests/test_output_formatting.py (2)
57-63: LGTM! Consider a slightly broader regex pattern for robustness.The helper function is well-documented with proper type hints. The current regex targets SGR sequences (ending in
m), which handles Rich's color and style codes. For broader ANSI coverage (cursor movement, clearing, etc.), consider:return re.sub(r"\x1b\[[0-9;]*[A-Za-z]", "", text)This is optional since Rich primarily emits SGR sequences for styling, and the current pattern should suffice for these tests.
97-129: Consider applyingstrip_ansiconsistently across similar tests.The
TestCxSteptests now usestrip_ansi, but other tests liketest_cx_print_info(lines 97-102) assert directly on raw output. If the current tests pass without stripping, this is fine, but applying the helper consistently would make all output formatting tests more resilient to styling changes.cortex/voice.py (1)
369-395: Add a return type for_get_hotkey_keyto satisfy typing rules.This keeps the new module aligned with the “type hints required” guideline. As per coding guidelines, type hints are required.
🧹 Suggested fix
- def _get_hotkey_key(self): + def _get_hotkey_key(self) -> Any:cortex/cli.py (1)
1527-1535: Remove unreachable execution block after the non-execute return.The block after
return 0is dead code; it can be removed or moved into the execute path if it was meant to run.🧹 Suggested fix
- print("\nExecuting commands...") - if parallel: - return self._handle_parallel_execution(commands, software, install_id, history) - - return self._handle_sequential_execution(commands, software, install_id, history)
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@cortex/cli.py`:
- Around line 850-875: In _install_with_session_key, remove the partial API key
leak from the debug log (the self._debug call that formats
api_key[:10]...api_key[-4:]) and replace it with a non-secret generic message
such as "Using session API key" or a masked constant like "<REDACTED_API_KEY>";
keep the provider log but never include any characters from api_key in logs and
ensure any future debug paths (e.g., in the same function) follow the same
masked convention.
In `@cortex/voice.py`:
- Around line 463-552: The docstrings for start_voice_mode and record_single
promise VoiceInputError/MicrophoneNotFoundError (and ModelNotFoundError) but the
current try/except blocks swallow these and return; change the handlers so the
exceptions propagate to callers: in start_voice_mode and record_single, let
exceptions from _ensure_dependencies, _check_microphone, and _load_model bubble
up (either remove the except blocks or re-raise after logging via cx_print), and
avoid returning "" on errors in record_single; still call stop() in finally
blocks where appropriate to clean up. Ensure references: start_voice_mode,
record_single, _ensure_dependencies, _check_microphone, _load_model, cx_print,
and stop.
♻️ Duplicate comments (1)
cortex/cli.py (1)
937-1167: Default empty input to dry-run in voice install prompts.Blank input currently loops/invalidates. To keep install flows dry-run by default, treat empty input as choice “1” in both prompt paths.
Based on learnings, ...✅ Suggested update
- return input("Enter choice [1/2/3]: ").strip() + choice = input("Enter choice [1/2/3]: ").strip() + return choice or "1"- choice = input("Enter choice [1/2/3]: ").strip() + choice = input("Enter choice [1/2/3]: ").strip() or "1"
🧹 Nitpick comments (2)
cortex/voice.py (1)
355-397: Add a return type hint for_get_hotkey_key.The method currently has no return annotation, which violates the type-hint requirement.
As per coding guidelines, ...♻️ Suggested update
- def _get_hotkey_key(self): + def _get_hotkey_key(self) -> Any:cortex/cli.py (1)
1569-1577: Remove unreachable execution block ininstall().The block that prints “Executing commands...” is unreachable due to the return just above it. This looks like leftover refactor code.
🧹 Suggested cleanup
- print("\nExecuting commands...") - if parallel: - return self._handle_parallel_execution(commands, software, install_id, history) - - return self._handle_sequential_execution(commands, software, install_id, history)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cortex/voice.py`:
- Around line 370-397: Add a return type annotation to _get_hotkey_key
indicating it returns either a pynput Key or None: change the signature to def
_get_hotkey_key(self) -> Optional['keyboard.Key']:, add from typing import
Optional to the module imports, and keep the local from pynput import keyboard
import inside the function (the quoted annotation avoids needing keyboard at
module scope).
♻️ Duplicate comments (1)
cortex/cli.py (1)
1382-1385: Remove partial API key logging.
Even partial key fragments are sensitive and should never hit logs (Line 1384).🔒 Proposed fix
- self._debug(f"API key: {api_key[:10]}...{api_key[-4:]}") + self._debug("API key loaded for session")
🧹 Nitpick comments (2)
cortex/cli.py (2)
1103-1135: Add a return type annotation toinput_handler_loop.
Type hints are required for Python functions. As per coding guidelines, please add explicit return types.♻️ Proposed change
- def input_handler_loop(): + def input_handler_loop() -> None:
1578-1586: Remove unreachable execution block after early return.
The block afterreturn 0is dead code and never runs.♻️ Proposed cleanup
else: print("\nTo execute these commands, run with --execute flag") print("Example: cortex install docker --execute") return 0 - - print("\nExecuting commands...") - if parallel: - return self._handle_parallel_execution(commands, software, install_id, history) - - return self._handle_sequential_execution(commands, software, install_id, history)
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@docs/VOICE_INPUT.md`:
- Around line 152-169: The docs incorrectly state Whisper models are single .pt
files; update the VOICE_INPUT.md section under "Uninstalling Models" to reflect
that cortex uses faster_whisper.WhisperModel with Hugging Face Hub caching so
models are stored as directories (not `{model_name}.pt`); remove or replace the
`rm ~/.cortex/models/base.en.pt` example, clarify that `rm -rf
~/.cortex/models/<model_dir>` or `ls ~/.cortex/models/` should be used to
remove/view models, and replace the "Model filename format" line with a note
that models are stored as directories named by the model repo/version rather
than single .pt files.
- Around line 171-180: The docs show voice config in ~/.cortex/config.yaml but
the code never reads it; update the CLI layer to load and parse that config and
pass values into VoiceInputHandler instead of relying solely on env
vars/defaults: add a config loader (e.g. reuse existing app config loader or
create a small YAML reader) that reads voice.model, voice.hotkey, and
voice.sample_rate from the config file and then instantiate
VoiceInputHandler(model_name=..., hotkey=..., sample_rate=...), preserving
current fallback order (env vars -> config file -> hardcoded defaults) and keep
existing env var handling (CORTEX_WHISPER_MODEL, CORTEX_VOICE_HOTKEY) so
behavior is unchanged if no config file exists; alternatively, if you prefer not
to implement config support, remove the config.yaml snippet from
docs/VOICE_INPUT.md so examples match current implementation.
🧹 Nitpick comments (1)
cortex/voice.py (1)
179-183: Invalid status value "dim" passed tocx_print.The
cx_printfunction inbranding.pyonly recognizes statuses: "info", "success", "warning", "error", "thinking". Using "dim" will silently fall back to "info", which may not be the intended visual style.♻️ Suggested fix
if info["desc"]: cx_print( f" {info['desc'].capitalize()} | Size: {size_str} | Tip: Use --model flag to try different models", - "dim", + "info", )Alternatively, if you want dimmed text, use
console.printdirectly with Rich markup.
Demolv_0_20260120000443.mp4Cannot add mic voice. Test locally on your system, try small.en or medium.en model for higher accuracy. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cortex/cli.py`:
- Around line 4584-4591: The ask command should not call cli.voice because that
flow is install-aware; when args.command == "ask" and getattr(args, "mic",
False) is true, capture speech and obtain a transcript directly and then call
cli.ask(transcript) (do not call cli.voice); update the handler to invoke the
existing speech-to-text helper or add a small helper that records/transcribes
audio and returns the text, then pass that text into cli.ask() so ask semantics
remain intact (refer to cli.voice, cli.ask and the args.mic flag to locate the
change).
♻️ Duplicate comments (1)
cortex/cli.py (1)
1382-1385: Don’t log any portion of the API key.
Even partial keys are sensitive and can leak via logs.🔒 Suggested fix
- self._debug(f"API key: {api_key[:10]}...{api_key[-4:]}") + self._debug("API key loaded for session")
🧹 Nitpick comments (4)
cortex/cli.py (4)
1103-1135: Add a return type annotation forinput_handler_loop.
This new helper should be fully typed for consistency.As per coding guidelines, please add the missing type hint.
♻️ Suggested tweak
- def input_handler_loop(): + def input_handler_loop() -> None:Please confirm this aligns with your typing/lint rules.
1209-1216: Annotateparallel_log_callbackreturn type.
Keeps new helpers fully typed.As per coding guidelines, please add the missing type hint.
♻️ Suggested tweak
- def parallel_log_callback(message: str, level: str = "info"): + def parallel_log_callback(message: str, level: str = "info") -> None:Please confirm this aligns with your typing/lint rules.
1292-1300: Type-hint the sequentialprogress_callback.
This new callback should include parameter + return annotations.As per coding guidelines, please add the missing type hints.
♻️ Suggested tweak
- def progress_callback(current, total, step): + def progress_callback(current: int, total: int, step: InstallationStep) -> None:Please confirm this aligns with your typing/lint rules.
1578-1586: Remove unreachable execution block after the early return.
This code never runs and duplicates the new helper paths.🧹 Suggested cleanup
- print("\nExecuting commands...") - if parallel: - return self._handle_parallel_execution(commands, software, install_id, history) - - return self._handle_sequential_execution(commands, software, install_id, history)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cortex/cli.py`:
- Around line 5361-5383: The VoiceInputHandler instance created in the --mic
branch (VoiceInputHandler(), referenced as handler) isn’t guaranteed to be
stopped/released on success or exception; wrap creation/use of handler and the
record_single() call in a try/finally and call the handler cleanup/stop method
(the same method used in the install --mic path or defined on VoiceInputHandler)
in the finally block so resources are always released whether transcript is
returned, an exception occurs (ImportError/VoiceInputError) or you return early.
🧹 Nitpick comments (1)
cortex/cli.py (1)
1579-1588: Remove unreachable execution block to avoid dead code.
The helper calls after the earlyreturn 0are never reached. Consider deleting or refactoring to use the helpers directly.♻️ Suggested cleanup
- print("\nExecuting commands...") - if parallel: - return self._handle_parallel_execution(commands, software, install_id, history) - - return self._handle_sequential_execution(commands, software, install_id, history)
|
@Anshgrover23 Ready for review all issue resolved and conversations as well. |
Anshgrover23
left a comment
Anshgrover23
left a comment
There was a problem hiding this comment.
@Sahilbhatane Kindly resolve conflicts.
|
4 participants
Related Issue
Closes #325
Summary
Adds voice input capability to Cortex Linux, enabling users to speak commands instead of typing them. This implementation uses local speech-to-text processing with Whisper for privacy and low latency.
Features Added
cortex voicefor continuous voice inputcortex voice --singlefor one-shot recordingcortex install --micandcortex ask --micCORTEX_VOICE_HOTKEY)● Recording...) during speech captureTechnical Details
faster-whisper(optimized Whisper) for accurate, local STTbase.en(~150MB, good accuracy/speed balance)pip install cortex-linux[voice]Files Changed
cortex/voice.py- New voice input handler modulecortex/cli.py- Added voice commands and --mic flagscortex/branding.py- Windows ASCII fallback for console outputpyproject.toml- Added[voice]optional dependenciesdocs/VOICE_INPUT.md- User documentationtests/test_voice.py- Unit tests (20 tests)tests/test_ollama_integration.py- Fixed Windows compatibilityChecklist
pytest tests/)ruff check,black --check)bandit -r cortex/)docs/VOICE_INPUT.md)additionally for Wayland based Ubuntu user (if hotkey doesn't work) -
Summary by CodeRabbit
New Features
Enhancements
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.