feat: Add requirements importer for multi-language dependency files (#126) #238
Conversation
…ortexlinux#126) Implements package import functionality for: - requirements.txt (Python/pip) - package.json (Node.js/npm) - Gemfile (Ruby/bundler) - Cargo.toml (Rust/cargo) - go.mod (Go modules) Features: - Auto-detection of requirements files - Dry-run mode for preview - Dev dependency support - Unified CLI interface - 50+ unit tests
WalkthroughThis PR introduces a unified multi-language dependency import and installation system for Cortex. It adds support for parsing Python requirements.txt, Node.js package.json, Ruby Gemfile, Rust Cargo.toml, and Go go.mod files, with corresponding CLI commands for detection, parsing, and package manager installation across all ecosystems. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as import_cli
participant Importer as RequirementsImporter
participant Parsers as Language Parsers
participant PackageManager as Package Manager<br/>(pip/npm/etc)
User->>CLI: cortex import file.txt [--all|--detect|--dry-run]
alt detect mode
CLI->>Importer: detect_files(dir)
Importer->>Importer: scan directory
Importer-->>CLI: list of requirement files
CLI-->>User: display detected files
else all mode
CLI->>Importer: parse_all(dir)
loop for each detected file
Importer->>Parsers: dispatch to language parser
Parsers->>Parsers: parse dependencies
Parsers-->>Importer: ParseResult
Importer->>Importer: accumulate results
end
Importer-->>CLI: all ParseResults
CLI-->>User: display per-file summary
alt dry-run
CLI-->>User: show what would install
else normal install
loop for each ParseResult
CLI->>Importer: install(result)
Importer->>PackageManager: run pip/npm/bundler/cargo/go
PackageManager-->>Importer: success/failure
Importer-->>CLI: result
end
CLI-->>User: installation complete
end
else single file mode
CLI->>Importer: parse_file(filepath)
Importer->>Parsers: dispatch to appropriate parser
Parsers->>Parsers: parse dependencies
Parsers-->>Importer: ParseResult
Importer-->>CLI: ParseResult
CLI-->>User: display dependencies
alt dry-run
CLI-->>User: show what would install
else normal install
CLI->>Importer: install(result)
Importer->>PackageManager: run appropriate installer
PackageManager-->>Importer: success/failure
Importer-->>CLI: result
CLI-->>User: installation complete
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 7
🧹 Nitpick comments (5)
cortex/import_cli.py (2)
80-178: Consider refactoring to reduce cognitive complexity.SonarCloud flags this function with cognitive complexity of 53 (limit: 15). While the logic is correct, extracting helper functions would improve maintainability.
Consider extracting the three modes into separate functions:
def _handle_detect_mode(importer: RequirementsImporter, directory: str) -> int: """Handle --detect mode.""" files = importer.detect_files(directory) if files: print("Detected requirements files:") for f in files: print(f" - {f}") else: print("No requirements files detected.") return 0 def _handle_import_all_mode(importer: RequirementsImporter, args: argparse.Namespace) -> int: """Handle --all mode.""" # ... existing import-all logic ... def _handle_single_file_mode(importer: RequirementsImporter, args: argparse.Namespace) -> int: """Handle single file import.""" # ... existing single-file logic ...Then
handle_importbecomes a simple dispatcher.
181-213: DRY violation: argument definitions are duplicated.The arguments defined in
main()(lines 193-204) duplicate those increate_import_parser()(lines 31-73). If arguments change, both locations must be updated.Consider extracting common argument configuration:
def _add_import_arguments(parser: argparse.ArgumentParser) -> None: """Add import-related arguments to a parser.""" parser.add_argument('file', nargs='?', help='Requirements file to import') parser.add_argument('--all', '-a', action='store_true', dest='import_all', ...) # ... other arguments ... parser.set_defaults(func=handle_import) def create_import_parser(subparsers): import_parser = subparsers.add_parser('import', ...) _add_import_arguments(import_parser) return import_parser def main(args=None): parser = argparse.ArgumentParser(prog='cortex-import', ...) _add_import_arguments(parser) # ... rest of main ...cortex/requirements_importer.py (3)
287-338: Consider using a TOML parser library for robustness.The hand-rolled TOML parsing may miss edge cases like:
- Multi-line strings
- Inline tables with nested brackets
- Comments within dependency declarations
Consider using
tomllib(Python 3.11+) ortomlifor proper TOML parsing:try: import tomllib except ImportError: import tomli as tomllib @staticmethod def parse(file_path: str) -> ParseResult: # ... with open(path, 'rb') as f: data = tomllib.load(f) for name, value in data.get('dependencies', {}).items(): version = value if isinstance(value, str) else value.get('version') # ...The current implementation works for common cases but may fail on complex Cargo.toml files.
480-520: Refactorparse_fileto reduce cognitive complexity.SonarCloud flags complexity of 17 (limit 15). The parser selection logic has redundant paths.
The current logic has overlapping conditions. Consider simplifying:
def parse_file(self, file_path: str) -> ParseResult: """Parse a single requirements file.""" filename = Path(file_path).name # Direct filename match if filename in self.PARSERS: parser = self.PARSERS[filename] # Requirements variants (requirements-*.txt, etc.) elif filename.startswith('requirements') and filename.endswith('.txt'): parser = RequirementsParser # Extension-based fallback else: parser = self._get_parser_by_extension(file_path) if parser is None: return ParseResult( dependencies=[], dev_dependencies=[], package_manager=PackageManager.PIP, source_file=file_path, errors=[f"Unknown file type: {file_path}"] ) result = parser.parse(file_path) self.results.append(result) return result def _get_parser_by_extension(self, file_path: str) -> Optional[type]: """Get parser based on file extension.""" ext_map = { '.txt': RequirementsParser, '.json': PackageJsonParser, '.toml': CargoTomlParser, '.mod': GoModParser, } for ext, parser in ext_map.items(): if file_path.endswith(ext): return parser if 'Gemfile' in file_path: return GemfileParser return None
558-559: Avoid catching blindException.Catching
Exceptioncan mask unexpected errors. Consider catching specific exceptions.- except Exception as e: + except (subprocess.SubprocessError, OSError, ValueError) as e: return False, str(e)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
README_IMPORT.md(1 hunks)cortex/import_cli.py(1 hunks)cortex/requirements_importer.py(1 hunks)tests/test_requirements_importer.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
cortex/import_cli.py (2)
cortex/requirements_importer.py (4)
PackageManager(23-29)detect_files(468-478)parse_file(480-520)install(533-559)src/sandbox_executor.py (1)
success(52-54)
tests/test_requirements_importer.py (1)
cortex/requirements_importer.py (19)
CargoTomlParser(287-376)Dependency(33-44)GemfileParser(221-284)GoModParser(379-446)PackageJsonParser(153-218)PackageManager(23-29)ParseResult(48-54)RequirementsImporter(449-648)RequirementsParser(57-150)parse(61-103)parse(157-211)parse(225-284)parse(291-338)parse(383-446)detect_files(468-478)parse_file(480-520)parse_all(522-531)install(533-559)summary(635-648)
🪛 GitHub Check: SonarCloud Code Analysis
cortex/import_cli.py
[failure] 80-80: Refactor this function to reduce its Cognitive Complexity from 53 to the 15 allowed.
[warning] 112-112: Remove the unused local variable "total_deps".
tests/test_requirements_importer.py
[warning] 590-590: Replace the unused local variable "message" with "_".
[warning] 610-610: Replace the unused local variable "message" with "_".
cortex/requirements_importer.py
[failure] 480-480: Refactor this function to reduce its Cognitive Complexity from 17 to the 15 allowed.
[failure] 383-383: Refactor this function to reduce its Cognitive Complexity from 20 to the 15 allowed.
[warning] 129-129: Rework this part of the regex to not match the empty string.
🪛 Ruff (0.14.7)
tests/test_requirements_importer.py
590-590: Unpacked variable message is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
610-610: Unpacked variable message is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
714-714: Unused method argument: capsys
(ARG002)
cortex/requirements_importer.py
131-131: Avoid specifying long messages outside the exception class
(TRY003)
243-243: Loop control variable line_num not used within loop body
Rename unused line_num to _line_num
(B007)
329-329: Do not catch blind exception: Exception
(BLE001)
401-401: Loop control variable line_num not used within loop body
Rename unused line_num to _line_num
(B007)
453-461: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
558-558: Do not catch blind exception: Exception
(BLE001)
572-572: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
576-576: subprocess call: check for execution of untrusted input
(S603)
585-585: Consider ['npm', 'install', *packages] instead of concatenation
Replace with ['npm', 'install', *packages]
(RUF005)
589-589: subprocess call: check for execution of untrusted input
(S603)
603-603: subprocess call: check for execution of untrusted input
(S603)
617-617: subprocess call: check for execution of untrusted input
(S603)
630-630: subprocess call: check for execution of untrusted input
(S603)
⏰ 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). (2)
- GitHub Check: Analyze (python)
- GitHub Check: Agent
🔇 Additional comments (10)
README_IMPORT.md (1)
1-235: Documentation is comprehensive and well-organized.The README covers all key aspects: CLI usage, supported formats, options, file format examples, Python API, and exit codes. The quick start section provides immediate value for users.
Minor suggestion: Consider adding a troubleshooting section for common issues (e.g., package manager not installed, permission errors).
cortex/import_cli.py (1)
22-77: LGTM!The argument parser is well-structured with appropriate short flags and help text. The use of
dest='import_all'to avoid conflict with Python'sallbuiltin is a good practice.tests/test_requirements_importer.py (5)
26-43: LGTM!Tests cover the essential Dependency dataclass behaviors including string representation with and without version, extras, and markers.
46-157: Good test coverage for Python requirements parsing.The tests cover key scenarios: simple requirements, versions, comments, extras, markers, empty lines, file not found, and recursive includes. The cleanup pattern is functional.
160-310: Good coverage for package.json and Gemfile parsers.Tests cover dependencies, devDependencies, groups, comments, empty files, invalid JSON, and file not found scenarios.
312-488: Good coverage for Cargo.toml and go.mod parsers.Tests cover simple dependencies, dev-dependencies, complex dependencies, require blocks, indirect dependencies, and edge cases.
728-762: Integration test provides good end-to-end validation.The test creates a realistic project structure with multiple dependency files and validates the full workflow: detection, parsing, and dry-run installation.
cortex/requirements_importer.py (3)
32-55: LGTM!Dataclasses are well-designed with proper use of
field(default_factory=list)for mutable default values.
57-150: Requirements parser implementation is solid.Good handling of:
- Version specifiers (==, >=, <=, >, <, ~=, !=)
- Extras (package[extra1,extra2])
- Environment markers
- Comments (line and inline)
- Recursive includes (with warning)
The regex on line 129 matching an empty remainder is intentional for packages without version specifiers.
561-592: Security consideration: subprocess calls with user-controlled input.Package names parsed from requirement files are passed directly to subprocess commands. A malicious package name could potentially exploit shell behavior.
While
subprocess.runwith a list of arguments (not shell=True) provides some protection, verify that:
- Package managers (pip, npm) properly validate package names
- No shell expansion occurs with special characters
The current implementation uses lists (not
shell=True), which is good. However, consider adding basic validation of package names:import re def _validate_package_name(name: str) -> bool: """Basic validation to reject obviously malicious package names.""" # Reject names with shell metacharacters if re.search(r'[;&|`$<>]', name): return False return TrueAlso, prefer unpacking as suggested by static analysis:
- cmd = [sys.executable, '-m', 'pip', 'install'] + packages + cmd = [sys.executable, '-m', 'pip', 'install', *packages]- cmd = ['npm', 'install'] + packages + cmd = ['npm', 'install', *packages]
| total_deps = len(result.dependencies) | ||
| if args.dev: | ||
| total_deps += len(result.dev_dependencies) |
There was a problem hiding this comment.
Remove unused variable total_deps.
The variable is computed but never used. As flagged by static analysis.
- total_deps = len(result.dependencies)
- if args.dev:
- total_deps += len(result.dev_dependencies)
-
print(f"\n{file_path} ({result.package_manager.value}):")🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 112-112: Remove the unused local variable "total_deps".
🤖 Prompt for AI Agents
In cortex/import_cli.py around lines 112 to 114, the variable total_deps is
computed but never used; remove the unused variable and its related increment
(delete the two lines that assign and modify total_deps) or, if a dependency
count is intended, replace its usage by actually using total_deps where needed
(e.g., in progress/output). Ensure no remaining references to total_deps remain
to avoid NameError.
| ) | ||
|
|
||
| with open(path, 'r') as f: | ||
| for line_num, line in enumerate(f, 1): |
There was a problem hiding this comment.
Rename unused loop variable.
The line_num variable is not used in the loop body.
- for line_num, line in enumerate(f, 1):
+ for _line_num, line in enumerate(f, 1):Or simply:
- for line_num, line in enumerate(f, 1):
+ for line in f:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for line_num, line in enumerate(f, 1): | |
| for _line_num, line in enumerate(f, 1): |
| for line_num, line in enumerate(f, 1): | |
| for line in f: |
🧰 Tools
🪛 Ruff (0.14.7)
243-243: Loop control variable line_num not used within loop body
Rename unused line_num to _line_num
(B007)
🤖 Prompt for AI Agents
In cortex/requirements_importer.py around line 243, the for-loop declares an
unused variable named line_num; rename it to _ (or _line_num) or remove it
entirely so the unused variable warning is resolved; update any references if
later used, otherwise iterate as for _, line in enumerate(f, 1): or simply for
line in f: to keep semantics identical.
| in_require_block = False | ||
|
|
||
| with open(path, 'r') as f: | ||
| for line_num, line in enumerate(f, 1): |
There was a problem hiding this comment.
Rename unused loop variable.
Same issue as GemfileParser - line_num is not used.
- for line_num, line in enumerate(f, 1):
+ for line in f:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for line_num, line in enumerate(f, 1): | |
| for line in f: |
🧰 Tools
🪛 Ruff (0.14.7)
401-401: Loop control variable line_num not used within loop body
Rename unused line_num to _line_num
(B007)
🤖 Prompt for AI Agents
In cortex/requirements_importer.py around line 401, the enumerate loop uses an
unused variable named line_num; rename it to _line_num or simply _ to avoid
unused-variable lint warnings and clarify intent (e.g., for line enumeration
that doesn’t use the index, change "for line_num, line in enumerate(f, 1):" to
"for _, line in enumerate(f, 1):" or "for _line_num, line in enumerate(f, 1):").
Ensure no other references to line_num remain.
| PARSERS = { | ||
| 'requirements.txt': RequirementsParser, | ||
| 'requirements-dev.txt': RequirementsParser, | ||
| 'requirements_dev.txt': RequirementsParser, | ||
| 'package.json': PackageJsonParser, | ||
| 'Gemfile': GemfileParser, | ||
| 'Cargo.toml': CargoTomlParser, | ||
| 'go.mod': GoModParser, | ||
| } |
There was a problem hiding this comment.
Annotate class variable with ClassVar.
PARSERS is a mutable class attribute and should be annotated with typing.ClassVar to indicate it's not an instance attribute.
+from typing import ClassVar, Dict, List, Optional, Tuple, Type
class RequirementsImporter:
"""Main class for importing and installing dependencies."""
# Mapping of file names to parsers
- PARSERS = {
+ PARSERS: ClassVar[Dict[str, Type]] = {
'requirements.txt': RequirementsParser,🧰 Tools
🪛 Ruff (0.14.7)
453-461: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
🤖 Prompt for AI Agents
In cortex/requirements_importer.py around lines 453 to 461, the PARSERS mutable
class attribute should be annotated as a ClassVar; update the declaration to
something like "PARSERS: ClassVar[Dict[str, Type[BaseParser]]]" (replace
BaseParser with the actual parser base class name used in the module) and add
"from typing import ClassVar, Dict, Type" to the imports if not present; leave
the dictionary value exactly the same but ensure the type annotation marks it as
a class-level variable.
| @patch('cortex.requirements_importer.subprocess.run') | ||
| def test_install_pip(self, mock_run): | ||
| mock_run.return_value = MagicMock(returncode=0, stdout="", stderr="") | ||
|
|
||
| with tempfile.NamedTemporaryFile(mode='w', suffix='.txt', delete=False) as f: | ||
| f.write("flask==2.0.1\nrequests\n") | ||
| f.flush() | ||
|
|
||
| importer = RequirementsImporter() | ||
| result = importer.parse_file(f.name) | ||
| success, message = importer.install(result) | ||
|
|
||
| os.unlink(f.name) | ||
|
|
||
| assert success | ||
| mock_run.assert_called_once() | ||
| call_args = mock_run.call_args[0][0] | ||
| assert 'pip' in ' '.join(call_args) | ||
| assert 'install' in call_args |
There was a problem hiding this comment.
Prefix unused variable with underscore.
The message variable is unpacked but never used. Prefix with underscore to indicate intentional discard.
- success, message = importer.install(result)
+ success, _message = importer.install(result)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @patch('cortex.requirements_importer.subprocess.run') | |
| def test_install_pip(self, mock_run): | |
| mock_run.return_value = MagicMock(returncode=0, stdout="", stderr="") | |
| with tempfile.NamedTemporaryFile(mode='w', suffix='.txt', delete=False) as f: | |
| f.write("flask==2.0.1\nrequests\n") | |
| f.flush() | |
| importer = RequirementsImporter() | |
| result = importer.parse_file(f.name) | |
| success, message = importer.install(result) | |
| os.unlink(f.name) | |
| assert success | |
| mock_run.assert_called_once() | |
| call_args = mock_run.call_args[0][0] | |
| assert 'pip' in ' '.join(call_args) | |
| assert 'install' in call_args | |
| @patch('cortex.requirements_importer.subprocess.run') | |
| def test_install_pip(self, mock_run): | |
| mock_run.return_value = MagicMock(returncode=0, stdout="", stderr="") | |
| with tempfile.NamedTemporaryFile(mode='w', suffix='.txt', delete=False) as f: | |
| f.write("flask==2.0.1\nrequests\n") | |
| f.flush() | |
| importer = RequirementsImporter() | |
| result = importer.parse_file(f.name) | |
| success, _message = importer.install(result) | |
| os.unlink(f.name) | |
| assert success | |
| mock_run.assert_called_once() | |
| call_args = mock_run.call_args[0][0] | |
| assert 'pip' in ' '.join(call_args) | |
| assert 'install' in call_args |
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 590-590: Replace the unused local variable "message" with "_".
🪛 Ruff (0.14.7)
590-590: Unpacked variable message is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🤖 Prompt for AI Agents
In tests/test_requirements_importer.py around lines 580 to 598, the test unpacks
the return of importer.install(result) into success, message but never uses
message; change the unpack to success, _message (or success, _ ) to indicate the
variable is intentionally unused and update the variable name in that assignment
only.
| @patch('cortex.requirements_importer.subprocess.run') | ||
| def test_install_npm(self, mock_run): | ||
| mock_run.return_value = MagicMock(returncode=0, stdout="", stderr="") | ||
|
|
||
| with tempfile.NamedTemporaryFile(mode='w', suffix='.json', delete=False) as f: | ||
| json.dump({"dependencies": {"express": "^4.0.0"}}, f) | ||
| f.flush() | ||
|
|
||
| importer = RequirementsImporter() | ||
| result = importer.parse_file(f.name) | ||
| success, message = importer.install(result) | ||
|
|
||
| os.unlink(f.name) | ||
|
|
||
| assert success | ||
| mock_run.assert_called_once() |
There was a problem hiding this comment.
Prefix unused variable with underscore.
Same issue as above - message is unpacked but not used.
- success, message = importer.install(result)
+ success, _message = importer.install(result)🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 610-610: Replace the unused local variable "message" with "_".
🪛 Ruff (0.14.7)
610-610: Unpacked variable message is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🤖 Prompt for AI Agents
In tests/test_requirements_importer.py around lines 600 to 615, the test unpacks
the tuple returned by importer.install(result) into (success, message) but never
uses message; change the unpack to (success, _message) or (success, _) to mark
the variable as intentionally unused (or rename to _message) so linters don't
complain, keeping the rest of the test the same.
| def test_verbose_mode(self, capsys): | ||
| importer = RequirementsImporter(verbose=True, dry_run=True) | ||
|
|
||
| with tempfile.NamedTemporaryFile(mode='w', suffix='.txt', delete=False) as f: | ||
| f.write("flask\n") | ||
| f.flush() | ||
|
|
||
| result = importer.parse_file(f.name) | ||
| importer.install(result) | ||
|
|
||
| os.unlink(f.name) | ||
| # Verbose mode should not crash, dry run prevents actual output |
There was a problem hiding this comment.
Remove unused capsys fixture or use it for assertion.
The capsys fixture is declared but never used. Either remove it or add assertions on captured output.
- def test_verbose_mode(self, capsys):
+ def test_verbose_mode(self):
importer = RequirementsImporter(verbose=True, dry_run=True)Alternatively, if you want to verify verbose output:
def test_verbose_mode(self, capsys):
importer = RequirementsImporter(verbose=True, dry_run=True)
# ... existing code ...
captured = capsys.readouterr()
# Add assertions on captured.out if verbose mode prints anything🧰 Tools
🪛 Ruff (0.14.7)
714-714: Unused method argument: capsys
(ARG002)
🤖 Prompt for AI Agents
In tests/test_requirements_importer.py around lines 714 to 725 the test function
declares the capsys fixture but never uses it; either remove the unused capsys
parameter from the test signature, or if you want to assert verbose output, keep
capsys and after importer.install(result) call capsys.readouterr() and add
assertions against captured.out (or captured.err) to verify expected verbose
messages; update the test accordingly and run tests to ensure no unused-fixture
warnings.
There was a problem hiding this comment.
Pull request overview
This PR implements a multi-language dependency file importer for Cortex, enabling users to import and install packages from requirements files across Python, Node.js, Ruby, Rust, and Go ecosystems. The implementation provides a unified CLI interface with dry-run capabilities, dev dependency support, and comprehensive error handling.
Key Changes
- Added
requirements_importer.pywith parsers for 5 package managers (pip, npm, bundler, cargo, go) with ~650 lines of parsing logic and installation methods - Implemented
import_cli.pyproviding a CLI interface with options for single-file import, bulk import, detection, and dry-run modes - Created 50+ unit tests covering parser functionality, edge cases, and integration scenarios
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 16 comments.
| File | Description |
|---|---|
| cortex/requirements_importer.py | Core implementation with parsers for each package manager format and installation logic |
| cortex/import_cli.py | CLI command handlers for import subcommand with argument parsing and output formatting |
| tests/test_requirements_importer.py | Comprehensive test suite covering all parsers, edge cases, and integration workflows |
| README_IMPORT.md | User documentation with usage examples, supported formats, and API reference |
Comments suppressed due to low confidence (1)
cortex/requirements_importer.py:493
- Test is always false, because of this condition.
elif filename == pattern:
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def test_dependency_str_with_version(self): | ||
| dep = Dependency(name="flask", version="2.0.1") | ||
| assert str(dep) == "flask==2.0.1" | ||
|
|
||
| def test_dependency_str_without_version(self): | ||
| dep = Dependency(name="flask") | ||
| assert str(dep) == "flask" |
There was a problem hiding this comment.
Missing test coverage for Dependency.__str__() when the version contains operators (e.g., ">=2.0.0", "~=1.0"). The current tests only cover exact versions ("2.0.1") and no version. Since the parsing logic stores versions with operators (as confirmed by test_parse_with_versions line 75-76), the __str__ method should be tested with these cases to catch the bug where it would produce invalid strings like "package==>=2.0.0".
|
|
||
| # Handle extras (e.g., package[extra1,extra2]) | ||
| extras = [] | ||
| extras_match = re.match(r'^([a-zA-Z0-9_-]+)\[([^\]]+)\](.*)$', line) |
There was a problem hiding this comment.
The regex pattern r'^([a-zA-Z0-9_-]+)\[([^\]]+)\](.*)$' for parsing extras doesn't properly handle package names with dots (e.g., zope.interface[test]). The character class should include dots: r'^([a-zA-Z0-9_.-]+)\[([^\]]+)\](.*)$'. This inconsistency exists because line 129 correctly allows dots in package names, but line 121 doesn't.
| extras_match = re.match(r'^([a-zA-Z0-9_-]+)\[([^\]]+)\](.*)$', line) | |
| extras_match = re.match(r'^([a-zA-Z0-9_.-]+)\[([^\]]+)\](.*)$', line) |
| def test_gemfile_complex_syntax(self): | ||
| with tempfile.NamedTemporaryFile(mode='w', delete=False) as f: | ||
| content = """ | ||
| source 'https://rubygems.org' | ||
|
|
||
| gem 'rails', '7.0.0' | ||
| gem 'pg', '>= 0.18', '< 2.0' | ||
| """ | ||
| f.write(content) | ||
| f.flush() | ||
|
|
||
| result = GemfileParser.parse(f.name) | ||
|
|
||
| os.unlink(f.name) | ||
|
|
||
| # Should parse both gems | ||
| assert len(result.dependencies) == 2 |
There was a problem hiding this comment.
The test test_gemfile_complex_syntax only verifies that 2 gems are parsed (line 712) but doesn't validate that the version constraints are correctly captured. Given the bug in the parser where only one version constraint is captured for gems with multiple constraints (e.g., gem 'pg', '>= 0.18', '< 2.0'), this test should be enhanced to verify the version field contains the expected constraint information.
| # Skip indirect dependencies | ||
| if '// indirect' not in line: |
There was a problem hiding this comment.
The go.mod parser checks for the exact string '// indirect' in the line to skip indirect dependencies (line 433). However, Go modules may use different comment styles or whitespace variations. Consider using a more robust check, such as checking if 'indirect' is in the comment portion after splitting by '//', or using regex to handle whitespace variations like '//indirect' or '// indirect '.
| # Skip indirect dependencies | |
| if '// indirect' not in line: | |
| # Skip indirect dependencies (robustly detect 'indirect' in comment) | |
| comment = '' | |
| if '//' in line: | |
| comment = line.split('//', 1)[1].strip().lower() | |
| if 'indirect' not in comment: |
|
|
||
| in_require_block = False | ||
|
|
||
| with open(path, 'r') as f: |
There was a problem hiding this comment.
File operations should specify encoding explicitly to avoid platform-dependent behavior. The open() calls on lines 76, 174, 242, and 400 don't specify an encoding parameter, which means they use the platform default encoding. This could cause issues when parsing files with non-ASCII characters (e.g., package names or comments with unicode characters). Consider adding encoding='utf-8' to all open() calls to ensure consistent behavior across platforms.
| with open(path, 'r') as f: | |
| with open(path, 'r', encoding='utf-8') as f: |
| def _install_npm(self, deps: List[Dependency]) -> Tuple[bool, str]: | ||
| """Install Node.js packages via npm.""" | ||
| packages = [f"{dep.name}@{dep.version}" if dep.version else dep.name for dep in deps] | ||
|
|
||
| cmd = ['npm', 'install'] + packages | ||
| if self.verbose: | ||
| print(f"Running: {' '.join(cmd)}") | ||
|
|
||
| result = subprocess.run(cmd, capture_output=True, text=True) | ||
| if result.returncode == 0: | ||
| return True, f"Installed {len(packages)} packages" | ||
| return False, result.stderr |
There was a problem hiding this comment.
The subprocess calls should include shell=False (which is the default) to prevent shell injection attacks. However, the commands constructed use user-controlled package names directly in the command arguments. While using a list (not shell=True) provides protection against command injection, there's no validation that package names don't contain special characters that could cause issues. Consider adding validation for package names to ensure they match expected patterns.
| packages.append(f"{dep.name}=={dep.version}" if not any( | ||
| op in dep.version for op in ['>=', '<=', '>', '<', '~=', '!='] | ||
| ) else f"{dep.name}{dep.version}") |
There was a problem hiding this comment.
[nitpick] In the _install_pip method, the version handling logic constructs different strings based on whether the version contains operators. However, the condition if not any(op in dep.version for op in ['>=', '<=', '>', '<', '~=', '!=']) results in f"{dep.name}=={dep.version}" when no operators are found, but f"{dep.name}{dep.version}" when operators are present. This means if a version is ">=2.0.0", the resulting string would be "package>=2.0.0" which is correct. However, if a version is stored as just "2.0.1" (extracted from "==2.0.1" on line 140), it would produce "package==2.0.1" which is also correct. The logic seems correct but the code flow on lines 139-143 where exact versions extract just the version number could lead to confusion since the "==" is stripped and re-added later.
| packages.append(f"{dep.name}=={dep.version}" if not any( | |
| op in dep.version for op in ['>=', '<=', '>', '<', '~=', '!='] | |
| ) else f"{dep.name}{dep.version}") | |
| packages.append(f"{dep.name}{dep.version}") |
|
|
||
| def __str__(self) -> str: | ||
| if self.version: | ||
| return f"{self.name}=={self.version}" |
There was a problem hiding this comment.
The __str__ method in the Dependency class always uses == for version formatting (line 43), but the version field can contain operators like >=, ~=, etc. (as seen in parsing logic on lines 142-143). This means calling str(dep) on a dependency with version ">=2.0.0" would produce "package==>=2.0.0" which is invalid. The __str__ method should handle version specifiers that already contain operators.
| return f"{self.name}=={self.version}" | |
| # If version starts with an operator, don't prepend '==' | |
| if self.version.startswith(("==", ">=", "<=", "~=", "!=", ">", "<")): | |
| return f"{self.name}{self.version}" | |
| else: | |
| return f"{self.name}=={self.version}" |
| PackageManager, | ||
| ) | ||
|
|
||
|
|
There was a problem hiding this comment.
Import of 'PackageManager' is not used.
| PackageManager, | |
| ) | |
| ) |
| import pytest | ||
|
|
There was a problem hiding this comment.
Import of 'pytest' is not used.
| import pytest |
|
@dhvll Could you review this PR? Your experience with the package manager wrapper would be helpful here. Thanks! |
2 participants
Summary
Implements package import functionality as requested in #126:
Features
cortex import <file>orcortex import --allcortex import --dry-runcortex import --devUsage
Test Plan
Files Added
cortex/requirements_importer.py- Core implementation (~650 lines)cortex/import_cli.py- CLI commands (~190 lines)tests/test_requirements_importer.py- Unit tests (~760 lines, 50 tests)README_IMPORT.md- DocumentationCloses #126
Summary by CodeRabbit
Release Notes
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.