feat: add package build from source (#101) #482
Conversation
Implements the ability to build and install packages from source code when binaries are unavailable. This feature includes automatic build dependency detection, build system detection, build caching, and comprehensive error handling.
|
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 end-to-end "build from source" support: new SourceBuilder module, CLI flags ( Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/CLI
participant CLI as cortex.cli
participant Builder as SourceBuilder
participant Cache as Build Cache
participant FS as Filesystem
participant PM as Package Manager
rect rgba(200,220,255,0.5)
User->>CLI: install --from-source pkg[`@version`] [--source-url URL] [--dry-run|--execute]
end
CLI->>CLI: _install_from_source(pkg, version, source_url, dry_run, execute)
CLI->>Builder: build_from_source(package_name, version, source_url)
Builder->>Cache: _get_cache_key(...) / _check_cache(...)
alt cache hit
Cache-->>Builder: cached build_dir + install_commands
Builder-->>CLI: BuildResult(cached=True)
else cache miss
Builder->>FS: fetch_source(url or detect)
FS-->>Builder: source_dir
Builder->>Builder: detect_build_system(source_dir)
Builder->>PM: detect_build_dependencies(build_system)
alt missing deps
Builder->>PM: request install missing_deps
PM-->>Builder: deps_installed
end
Builder->>Builder: configure_build(source_dir)
Builder->>Builder: build(source_dir)
Builder->>Builder: install_build(source_dir)
Builder->>Cache: _save_to_cache(key, build_dir, commands)
Builder-->>CLI: BuildResult(success=True, install_commands)
end
alt execute
CLI->>PM: run install_commands
PM-->>CLI: success/failure
else dry-run
CLI-->>User: show install_commands (no exec)
end
CLI->>User: report result & history update
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cortex/cli.py (1)
1-2050: Address Black formatting failure.The CI pipeline indicates that Black formatting check failed with "1 file would be reformatted." Run
black cortex/cli.pyto fix the formatting issues before merging.
🧹 Nitpick comments (6)
docs/SOURCE_BUILD.md (1)
78-82: Consider notingsetup.pydeprecation in documentation.The documentation mentions
python3 setup.py buildandsudo python3 setup.py installfor Python builds. While this still works for legacy projects, modern Python packaging preferspip install .orpython -m buildwithpyproject.toml. Consider adding a note about this or supporting the modern approach as well.tests/test_source_builder.py (2)
184-209: Consider adding stronger assertions for fetch test.The test mocks the tarfile extraction but only verifies
result is not None. Consider asserting on the returned path structure to ensure the extraction logic correctly identifies the source directory.🔎 Suggested improvement
result = self.builder._fetch_from_url( "https://example.com/test.tar.gz", "test", "1.0" ) assert result is not None + # Verify returned path is a valid directory + assert isinstance(result, Path)
235-253: Strengthen assertion for success test.The test comment states "Should succeed" but only asserts
result is not None. Add an assertion forresult.successto properly verify the expected outcome.🔎 Proposed fix
result = self.builder.build_from_source("test-package", use_cache=False) - # Should succeed (or at least not fail on dependency check) - assert result is not None + # Should succeed since dependencies are satisfied and commands succeed + assert result is not None + assert result.success is Truecortex/source_builder.py (3)
36-37: Module-level side effect on import.
CACHE_DIR.mkdir(parents=True, exist_ok=True)executes at import time, which could cause issues in testing or when the module is imported but not used. Consider moving this to theSourceBuilder.__init__method (which already has a similar call at line 96).🔎 Proposed fix
# Build cache directory CACHE_DIR = Path.home() / ".cortex" / "build_cache" -CACHE_DIR.mkdir(parents=True, exist_ok=True)The directory creation in
__init__at line 96 already handles this.
194-200: GitHub tag URL assumes 'v' prefix for versions.Not all GitHub projects use the
vprefix for version tags (e.g., some use1.0.0instead ofv1.0.0). Consider making this configurable or attempting both patterns.
185-225: Temporary directories not cleaned up after use.The temp directory created at line 187 is never explicitly cleaned up. While the OS may eventually clean
/tmp, builds could accumulate significant disk space. Consider adding cleanup after the install commands are executed, or using a context manager pattern.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cortex/cli.pycortex/source_builder.pydocs/SOURCE_BUILD.mdpyproject.tomltests/test_source_builder.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_source_builder.pycortex/source_builder.pycortex/cli.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_source_builder.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 (2)
cortex/source_builder.py (3)
cortex/branding.py (1)
cx_print(49-69)cortex/dependency_resolver.py (2)
DependencyResolver(40-386)is_package_installed(101-104)cortex/utils/commands.py (1)
CommandResult(118-125)
cortex/cli.py (2)
cortex/coordinator.py (4)
execute(230-279)InstallationCoordinator(51-324)InstallationStep(26-39)StepStatus(17-22)cortex/source_builder.py (2)
SourceBuilder(81-525)build_from_source(382-525)
🪛 GitHub Actions: CI
cortex/cli.py
[error] 1-1: Black formatting check failed. 1 file would be reformatted by Black. Run 'black .' to format.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test (3.11)
- GitHub Check: test (3.10)
- GitHub Check: test (3.12)
🔇 Additional comments (6)
pyproject.toml (1)
137-137: LGTM!Adding
docsto the Ruff exclude list is appropriate since the documentation directory contains Markdown files, not Python code.cortex/cli.py (1)
1549-1598: Well-structured source build integration.The
_install_from_sourcemethod properly integrates withSourceBuilder, handles caching, and supports both dry-run and execute modes. The progress callback follows existing patterns in the codebase.tests/test_source_builder.py (2)
1-21: LGTM - Well-structured test setup.Imports are properly organized and the test file follows pytest conventions. The import of
BUILD_DEPENDENCIES,BuildConfig,BuildResult, andSourceBuilderaligns with the public API of the source_builder module.
23-253: Good test coverage for SourceBuilder.The
TestSourceBuilderclass provides comprehensive coverage of key functionality including initialization, cache key generation, dependency detection for multiple build systems, build system detection, and the build-from-source workflow. The use oftempfile.TemporaryDirectoryfor filesystem tests is appropriate.cortex/source_builder.py (2)
53-78: LGTM - Well-defined dataclasses.
BuildConfigandBuildResultare properly typed with sensible defaults. The use of@dataclasskeeps the code clean and provides automatic__init__,__repr__, etc.
382-525: Well-implemented build orchestration.The
build_from_sourcemethod properly orchestrates the entire build workflow: cache check, source fetch, dependency detection, configure, build, and install command generation. Error handling is comprehensive with informative error messages inBuildResult. The integration withDependencyResolverandrun_commandutility follows existing patterns.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
docs/SOURCE_BUILD.md (1)
237-242: Verify that referenced documentation files exist.The "See Also" section references documentation files that may not exist in the repository. A previous review flagged that
Getting-Started.mdandREADME_DEPENDENCIES.mdwere missing. Please verify these files exist or update the references to point to existing documentation.
🧹 Nitpick comments (5)
cortex/cli.py (2)
554-563: Update docstring to document new parameters.The method signature was extended with
from_source,source_url, andversionparameters, but the docstring was not updated to describe them. Adding parameter documentation would improve maintainability.🔎 Proposed enhancement
Add to the method's docstring:
"""Install software using natural language or from source. Args: software: Software description or package name execute: Whether to execute installation commands dry_run: Show commands without executing parallel: Enable parallel execution for multi-step installs from_source: Build and install from source code when True source_url: URL to source code (for from_source builds) version: Version to build (for from_source builds) Returns: Exit code (0 for success, 1 for failure) """
1547-1624: Consider adding audit logging for source builds.Based on learnings, all package operations should implement audit logging to
~/.cortex/history.db. The_install_from_sourcemethod doesn't record installation attempts to the history database, which means source builds won't appear incortex historyor be available for rollback.Consider integrating with
InstallationHistorysimilar to the regularinstall()flow (lines 592-627 and 754-757).Based on learnings, audit logging should be implemented for install operations.
cortex/source_builder.py (3)
110-125: Consider error handling for build_dir stat.Line 119 calls
Path(build_dir).stat().st_mtimewhich could raiseFileNotFoundErrorif the build directory was removed between build and cache save. While unlikely in normal operation, adding a try-except or verification would improve robustness.🔎 Optional enhancement
# Save metadata + try: + timestamp = str(Path(build_dir).stat().st_mtime) + except (OSError, FileNotFoundError): + timestamp = str(time.time()) metadata = { "build_dir": str(build_dir), "install_commands": install_commands, - "timestamp": str(Path(build_dir).stat().st_mtime), + "timestamp": timestamp, }
209-220: Enhance zipfile path traversal protection.The current check for
".."and"/"prefix in zip members provides basic protection, but could be bypassed by certain path constructions. Consider usingPath.resolve()to verify that extracted paths remain within the target directory.🔎 Enhanced security check
elif archive_path.suffix == ".zip": with zipfile.ZipFile(archive_path, "r") as zip_ref: # Filter out path traversal components for security for member in zip_ref.namelist(): - # Skip files with path traversal or absolute paths - if ".." in member or member.startswith("/"): - continue - zip_ref.extract(member, extract_dir) + # Verify extraction path is within target directory + target_path = (extract_dir / member).resolve() + if not target_path.is_relative_to(extract_dir.resolve()): + logger.warning(f"Skipping unsafe path in archive: {member}") + continue + zip_ref.extract(member, extract_dir)Note:
is_relative_to()requires Python 3.9+, which is supported perpyproject.toml.
335-357: Consider extracting parallel job count logic.The code for determining parallel job count using
multiprocessing.cpu_count()is duplicated in lines 341-343 and 353-355. Extracting this to a helper or computing once would reduce duplication.🔎 Optional refactor
def build(self, source_dir: Path, config: BuildConfig) -> list[str]: """Build the package.""" commands = [] + + # Determine parallel jobs + import multiprocessing + jobs = multiprocessing.cpu_count() if config.build_system == "autotools" or config.build_system == "make": make_cmd = "make" if config.make_args: make_cmd += " " + " ".join(config.make_args) else: - import multiprocessing - jobs = multiprocessing.cpu_count() make_cmd += f" -j{jobs}" commands.append(make_cmd) elif config.build_system == "cmake": build_dir = source_dir / "build" make_cmd = "make" if config.make_args: make_cmd += " " + " ".join(config.make_args) else: - import multiprocessing - jobs = multiprocessing.cpu_count() make_cmd += f" -j{jobs}" commands.append(f"cd {build_dir} && {make_cmd}")
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cortex/cli.pycortex/source_builder.pydocs/SOURCE_BUILD.md
🧰 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/source_builder.py
🧠 Learnings (1)
📚 Learning: 2025-12-11T12:03:24.071Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T12:03:24.071Z
Learning: Applies to **/*install*.py : Implement audit logging to ~/.cortex/history.db for all package operations
Applied to files:
cortex/cli.py
🧬 Code graph analysis (2)
cortex/cli.py (1)
cortex/source_builder.py (1)
build_from_source(388-531)
cortex/source_builder.py (3)
cortex/branding.py (1)
cx_print(49-69)cortex/dependency_resolver.py (2)
DependencyResolver(40-386)is_package_installed(101-104)cortex/utils/commands.py (1)
CommandResult(118-125)
⏰ 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). (5)
- GitHub Check: Test (Python 3.12)
- GitHub Check: Test (Python 3.10)
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
- GitHub Check: test (3.10)
🔇 Additional comments (6)
cortex/cli.py (2)
1745-1759: CLI arguments properly defined.The new command-line arguments for source builds are well-structured with appropriate types and clear help text. The
--pkg-versionnaming avoids the previously identified conflict with the global--versionflag.
1995-2004: Proper wiring of source build parameters.The new parameters are correctly passed from command-line arguments to the
install()method using defensivegetattrcalls with appropriate defaults.cortex/source_builder.py (4)
53-78: Well-designed data structures.The
BuildConfigandBuildResultdataclasses are cleanly defined with appropriate type hints and sensible defaults. They provide a clear interface for build operations.
127-160: Dependency detection logic is sound.The method correctly aggregates build dependencies based on the build system and package patterns, then filters to return only missing packages. The heuristic for Python-related packages is reasonable.
255-282: Build system detection is well-structured.The detection logic checks for build system indicators in a sensible order and provides a reasonable default. The approach covers the most common build systems.
388-531: Well-orchestrated build workflow.The
build_from_sourcemethod provides a comprehensive build pipeline with:
- Proper caching to avoid redundant builds
- Automatic dependency detection and installation
- Build system auto-detection
- Progressive user feedback via
cx_print- Appropriate timeouts for different build stages
- Comprehensive error handling with descriptive messages
The structure is clean and maintainable.
Replace filter='data' parameter (Python 3.12+) with manual member filtering for Python 3.10/3.11 compatibility. Still prevents path traversal attacks. Fixes test failures on Python 3.10 and 3.11 in CI
Update test_fetch_from_url_tarball to mock getmembers() and handle the members parameter in extractall() call for Python 3.10+ compatibility. Fixes test failure: mock_extractall() got an unexpected keyword argument 'members'
- Mock urlretrieve to create dummy archive file - Ensure mock_extractall creates structure in correct extract_dir - Add Path type assertion to test - Remove unnecessary tempdir setup that doesn't match implementation Fixes test failure where mock wasn't creating structure in the extract_dir that _fetch_from_url actually uses.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_cli.py (1)
207-253: Add integration tests for new source-build CLI flags to meet >80% coverage requirement.The PR introduces new CLI flags (
--from-source,--source-url,--pkg-version) that are correctly implemented in the argument parser and install method signature, but the integration tests at lines 207-253 do not exercise them. The existing tests only verify the base case,--execute, and--dry-runflags.Add the following test cases to cover the new functionality:
Suggested test cases
@patch("sys.argv", ["cortex", "install", "docker", "--from-source"]) @patch("cortex.cli.CortexCLI.install") def test_main_install_with_from_source(self, mock_install): mock_install.return_value = 0 result = main() self.assertEqual(result, 0) mock_install.assert_called_once_with( "docker", execute=False, dry_run=False, parallel=False, from_source=True, source_url=None, version=None, ) @patch("sys.argv", ["cortex", "install", "docker", "--from-source", "--source-url", "https://example.com/package.tar.gz"]) @patch("cortex.cli.CortexCLI.install") def test_main_install_with_source_url(self, mock_install): mock_install.return_value = 0 result = main() self.assertEqual(result, 0) mock_install.assert_called_once_with( "docker", execute=False, dry_run=False, parallel=False, from_source=True, source_url="https://example.com/package.tar.gz", version=None, ) @patch("sys.argv", ["cortex", "install", "docker", "--pkg-version", "1.0.0"]) @patch("cortex.cli.CortexCLI.install") def test_main_install_with_pkg_version(self, mock_install): mock_install.return_value = 0 result = main() self.assertEqual(result, 0) mock_install.assert_called_once_with( "docker", execute=False, dry_run=False, parallel=False, from_source=False, source_url=None, version="1.0.0", )The coding guidelines require >80% test coverage for
tests/**/*.pyfiles. Verify coverage withpython -m pytest tests/test_cli.py --cov=cortex.cli --cov-report=term-missingafter adding these tests.
🧹 Nitpick comments (1)
cortex/source_builder.py (1)
356-395: Movemultiprocessingimport to module level.The
multiprocessingmodule is imported inline twice within the same method (lines 374 and 386). Per PEP 8, imports should be at the top of the file unless there's a specific reason for lazy loading.🔎 Suggested refactor
At the top of the file (after line 27):
import zipfile +import multiprocessing from dataclasses import dataclassThen remove the inline imports and use
multiprocessing.cpu_count()directly:if config.build_system == "autotools" or config.build_system == "make": make_cmd = "make" if config.make_args: make_cmd += " " + " ".join(config.make_args) else: # Use parallel builds by default - import multiprocessing - jobs = multiprocessing.cpu_count() make_cmd += f" -j{jobs}"Apply the same change at line 386.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cortex/source_builder.pytests/test_cli.pytests/test_source_builder.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_source_builder.pytests/test_cli.pycortex/source_builder.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_source_builder.pytests/test_cli.py
🧬 Code graph analysis (3)
tests/test_source_builder.py (1)
cortex/source_builder.py (11)
BuildConfig(54-65)BuildResult(69-78)SourceBuilder(81-564)_get_cache_key(98-101)detect_build_dependencies(127-160)detect_build_system(288-315)configure_build(317-354)build(356-395)install_build(397-419)_fetch_from_url(185-264)build_from_source(421-564)
tests/test_cli.py (1)
cortex/cli.py (1)
main(1680-2041)
cortex/source_builder.py (3)
cortex/branding.py (1)
cx_print(49-69)cortex/dependency_resolver.py (1)
is_package_installed(101-104)cortex/utils/commands.py (1)
CommandResult(118-125)
⏰ 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: Test (Python 3.12)
- GitHub Check: test (3.12)
- GitHub Check: test (3.11)
- GitHub Check: test (3.10)
🔇 Additional comments (5)
tests/test_cli.py (3)
213-221: LGTM! Assertion correctly updated for extended signature.The test assertion properly includes the three new keyword arguments (
from_source,source_url,version) with their expected default values for a basic install command.
229-237: LGTM! Assertion correctly updated for extended signature.The assertion properly verifies that the
--executeflag is handled independently of the new source-build parameters, with all new arguments correctly set to their defaults.
245-253: LGTM! Assertion correctly updated for extended signature.The assertion properly verifies that the
--dry-runflag is handled independently of the new source-build parameters, with all new arguments correctly set to their defaults.cortex/source_builder.py (1)
209-253: LGTM: Robust path traversal protection implemented.The manual filtering for both tarfile and zipfile extraction properly prevents CVE-2007-4559 and similar zip slip attacks. The implementation correctly:
- Rejects absolute paths and
..components- Resolves and validates each member path stays within
extract_dir- Handles Python 3.10/3.11 compatibility (where
filter='data'isn't available)tests/test_source_builder.py (1)
184-228: LGTM: Thorough mocking for tarball extraction test.The test properly mocks the complex tarfile extraction flow, including:
- URL retrieval simulation
- Member filtering (aligning with the security checks in the implementation)
- Creating actual directory structure to match extraction behavior
- Handling Path vs. string arguments
This validates the security-enhanced extraction logic while maintaining test isolation.
aybanda
force-pushed
the
feature/package-build-from-source--101
branch
from
c267eb5 to
a471a6a
Compare
CLA Verification PassedAll contributors have signed the CLA.
|
Anshgrover23
left a comment
Anshgrover23
left a comment
There was a problem hiding this comment.
@aybanda CI check is failing.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @cortex/cli.py:
- Around line 1625-1701: Add audit logging to the _install_from_source flow
mirroring the install() method's history writes: open the same history DB used
in install() (e.g., the History/HistoryDB helper used at lines ~670-705) and
record an entry for this source-based install with package_name, version (if
any), source_url (mark source='source' or include URL), timestamp, success
boolean and any error_message; do this after a successful install_result.success
write a success entry, and on failures write a failed entry (including
install_result.error_message or result.error_message), ensuring the code path
around _install_from_source / InstallationCoordinator / install_result uses the
same history API and file (~/.cortex/history.db) as install().
In @cortex/source_builder.py:
- Around line 36-38: Remove the module-level directory creation (the
CACHE_DIR.mkdir call) to avoid side effects at import; keep the CACHE_DIR =
Path.home() / ".cortex" / "build_cache" constant but defer creating it to
runtime in SourceBuilder.__init__. In SourceBuilder.__init__ (the constructor
that already attempts to create the cache), wrap the mkdir logic in a
try/except, handle exceptions by falling back to a safe alternative (e.g., set
self.cache_dir to None or a temp directory) and emit a logged warning/error so
the rest of the class can operate without failing the import.
- Around line 184-206: In _fetch_from_url, validate the incoming url string
before calling urllib.request.urlretrieve: parse the URL (urllib.parse.urlparse)
and ensure the scheme is either http or https (reject file:, data:, ftp:, etc.),
then resolve the hostname to IPs and use the ipaddress module to reject
loopback, link‑local, private RFC1918 ranges, and known metadata addresses
(e.g., 169.254.169.254); if any check fails, raise a ValueError or custom
exception. Apply this validation at the start of _fetch_from_url (before
creating archive_path) and use the same url variable when modifying GitHub paths
so that only validated, safe URLs reach urllib.request.urlretrieve.
🧹 Nitpick comments (4)
tests/test_source_builder.py (1)
196-239: Consider simplifying the fetch test mocking.The test uses complex mocking with a nested
mock_extractallfunction and nonlocal variables. While functional, this could be more maintainable.🔎 Alternative approach using a simpler mock structure
Consider using
side_effectdirectly on the tar object methods:def test_fetch_from_url_tarball(self, mock_tarfile, mock_urlretrieve, mock_run_command): """Test fetching source from URL (tarball).""" def mock_urlretrieve_impl(url, filepath): Path(filepath).parent.mkdir(parents=True, exist_ok=True) Path(filepath).touch() mock_urlretrieve.side_effect = mock_urlretrieve_impl # Create a simple mock that simulates extraction mock_tar = MagicMock() mock_member = MagicMock() mock_member.name = "source-1.0" mock_tar.getmembers.return_value = [mock_member] # Mock extractall to create the directory structure def create_extracted_dir(path, members=None): extract_path = Path(path) (extract_path / "source-1.0").mkdir(parents=True, exist_ok=True) (extract_path / "source-1.0" / "README").touch() mock_tar.extractall.side_effect = create_extracted_dir mock_tarfile.return_value.__enter__.return_value = mock_tar result = self.builder._fetch_from_url("https://example.com/test.tar.gz", "test", "1.0") assert result.name == "source-1.0" assert result.is_dir()cortex/source_builder.py (3)
94-97: Redundant cache directory creation.The cache directory is already created at module level (line 38), making the
mkdircall at line 97 redundant. However, if you address the module-level side effect issue (see previous comment), this line would become necessary.
99-102: Consider longer cache keys to reduce collision risk.The cache key is truncated to 16 hex characters (64 bits), which provides adequate collision resistance for typical use cases but may be insufficient for very large cache populations. Consider using at least 32 characters (128 bits) for better collision resistance in production environments.
🔎 Proposed fix
def _get_cache_key(self, package_name: str, version: str | None, source_url: str) -> str: """Generate a cache key for a build.""" key_data = f"{package_name}:{version}:{source_url}" - return hashlib.sha256(key_data.encode()).hexdigest()[:16] + return hashlib.sha256(key_data.encode()).hexdigest()[:32]
406-409: Importmultiprocessingat module level for efficiency.The
multiprocessingmodule is imported inside thebuildmethod at two different locations (lines 406-409 and 420-423), which is inefficient. Module imports should generally be done at the top of the file for better performance and clarity.🔎 Proposed fix
Add to the imports at the top of the file (after line 20):
import shutil import subprocess import tarfile import tempfile import urllib.request import zipfile +import multiprocessing from dataclasses import dataclassThen remove the local imports and use
multiprocessing.cpu_count()directly at lines 408 and 422.Also applies to: 420-423
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/codeql.ymlcortex/cli.pycortex/source_builder.pycortex/utils/commands.pytests/test_cli_extended.pytests/test_source_builder.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_source_builder.pycortex/utils/commands.pytests/test_cli_extended.pycortex/source_builder.pycortex/cli.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_source_builder.pytests/test_cli_extended.py
🧠 Learnings (1)
📚 Learning: 2025-12-11T12:03:24.071Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T12:03:24.071Z
Learning: Applies to **/*install*.py : Implement audit logging to ~/.cortex/history.db for all package operations
Applied to files:
cortex/cli.py
🧬 Code graph analysis (3)
tests/test_source_builder.py (1)
cortex/source_builder.py (10)
BuildConfig(55-66)BuildResult(70-79)_get_cache_key(99-102)detect_build_dependencies(128-161)detect_build_system(296-323)configure_build(325-382)build(384-431)install_build(433-455)_fetch_from_url(184-274)build_from_source(457-614)
tests/test_cli_extended.py (2)
tests/test_cli.py (2)
test_main_install_with_execute(225-237)test_main_install_with_dry_run(241-253)cortex/cli.py (1)
main(1765-2151)
cortex/source_builder.py (7)
cortex/branding.py (1)
cx_print(49-69)cortex/dependency_resolver.py (2)
DependencyResolver(40-386)is_package_installed(101-104)cortex/utils/commands.py (3)
CommandResult(120-127)run_command(223-305)validate_command(136-198)cortex/sandbox/sandbox_executor.py (1)
success(74-76)cortex/kernel_features/llm_device.py (1)
open(158-159)tests/test_env_manager.py (1)
temp_dir(44-47)cortex/env_manager.py (1)
load(571-595)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test (3.10)
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
🔇 Additional comments (23)
.github/workflows/codeql.yml (1)
28-31: LGTM!The CodeQL workflow updates are appropriate. Using default queries and adding Autobuild aligns with GitHub's recommended CodeQL configuration and should help avoid conflicts with default setups.
tests/test_cli_extended.py (1)
306-314: LGTM!The test assertions are correctly updated to reflect the new
install()method signature withfrom_source,source_url, andversionparameters. The default values (False, None, None) appropriately represent the behavior when these flags are not provided.Also applies to: 322-330, 338-346
tests/test_source_builder.py (8)
26-33: LGTM!The test initialization properly sets up the SourceBuilder instance and verifies that the dependency resolver and cache directory are correctly configured.
35-46: LGTM!The cache key generation test thoroughly validates:
- Deterministic key generation (same inputs → same key)
- Differentiation (different inputs → different key)
- Key format (16 characters as specified)
48-82: LGTM!The dependency detection tests provide excellent coverage across build systems (autotools, cmake, python) and include an important edge case test for already-satisfied dependencies. The mocking strategy properly isolates the package manager checks.
84-117: LGTM!The build system detection tests cover all supported systems (autotools, cmake, make, python) plus the default fallback case. Using actual file creation in temporary directories provides better confidence than pure mocking.
119-152: LGTM!The configure build tests validate:
- Command structure (bash prefix for autotools, cmake for cmake)
- Default arguments (--prefix for autotools, CMAKE_INSTALL_PREFIX for cmake)
- Working directory handling (important for cmake builds)
The tuple return format
(command, working_dir)is properly tested.
241-286: LGTM!The integration tests cover both failure and success paths for the complete build-from-source workflow:
- Dependency installation failure handling
- Successful end-to-end build with all steps coordinated
- Proper mocking of the entire pipeline
The success test properly disables caching (
use_cache=False) to ensure deterministic behavior.
289-316: LGTM!The BuildConfig dataclass tests validate both default values and custom configurations, ensuring the configuration object behaves as expected.
319-341: LGTM!The BUILD_DEPENDENCIES constant tests ensure the dependency structure is correctly defined and contains expected packages for each build system category.
cortex/cli.py (5)
638-640: LGTM!The new parameters for source-based installation are well-designed:
from_source: Boolean flag with appropriate default (False)source_url: Optional URL for custom source locationsversion: Optional version specificationThe type hints are correct and the parameter names are clear.
675-677: LGTM!The early return for the
--from-sourceflow is a clean way to separate the source-building logic from the normal installation path. This avoids complex conditional logic and keeps the main install method focused.
1625-1642: Good edge case handling for version parsing.The version parsing correctly handles the edge case where a user provides
package@(with no version after the@) by checking bothlen(parts) > 1andparts[1]before assigning. This addresses the concern from previous reviews.
1846-1860: LGTM!The CLI argument definitions are well-structured:
--from-source: Clear boolean flag for enabling source builds--source-url: Optional URL parameter for custom source locations--pkg-version: Renamed to avoid conflict with global--versionflag (addresses previous review feedback)The help text is descriptive and guides users appropriately.
2111-2113: LGTM!The parameter passing uses
getattr()with appropriate defaults to safely handle the new CLI arguments and correctly mapsargs.pkg_versionto theversionparameter.cortex/utils/commands.py (1)
114-115: The bash/sh allowlist addition is properly constrained and safe.The security concern is well-mitigated by the implementation:
- All bash/sh invocations use
shlex.quote()for paths and arguments (source_builder.py lines 353-372)- Commands are executed via
shlex.split()(commands.py line 272), which parses into safe argument arrays rather than shell interpretation- The
validate_command()function blocks dangerous patterns and shell metacharacters before execution- Even allowed characters like
&&and||won't execute as operators because they're treated as literal string arguments byshlex.split()No changes needed; the implementation properly defends against injection.
cortex/source_builder.py (7)
54-80: LGTM! Well-structured dataclasses.The
BuildConfigandBuildResultdataclasses are well-designed with proper type hints (using Python 3.10+ syntax) and clear docstrings. The fields appropriately capture build configuration and results.
213-257: LGTM! Comprehensive path traversal protection.The manual filtering for both tarfile and zipfile extraction properly prevents path traversal attacks (CVE-2007-4559 and similar). The implementation correctly:
- Rejects paths with
..components or absolute paths- Resolves paths and validates they remain within the extraction directory
- Handles exceptions gracefully
This addresses the security concerns from previous reviews.
296-323: LGTM! Build system detection logic is sound.The detection order (autotools → cmake → make → python) is appropriate, and the fallback to "autotools" is documented in the docstring. The file checks are straightforward and correct.
325-382: LGTM! Command generation now properly handles validation requirements.The refactored
configure_buildmethod addresses previous command validation concerns by:
- Using
bash(an allowed command prefix) to execute configure scripts instead of./configure- Properly sanitizing arguments with
shlex.quote- Returning tuples of
(command, working_directory)to avoid shell operators likecd ... &&- Using CMake's
-Bstyle with relative paths and working directory contextThis resolves the validation failures mentioned in past reviews.
457-614: LGTM! Well-orchestrated build workflow with comprehensive error handling.The
build_from_sourcemethod properly orchestrates the complete build process with:
- Cache checking to avoid redundant builds
- Source fetching with error handling
- Build system detection
- Dependency resolution and installation
- Phased execution (configure → build → install command generation)
- Proper timeout values (5min for configure, 1hr for build)
- Comprehensive error handling that returns
BuildResultwith detailed error messages- User-friendly progress messages via
cx_printThe dependency installation at lines 532-533 is safe because
missing_depscomes from the controlledBUILD_DEPENDENCIESdictionary.
433-455: No action needed — install commands are executed throughInstallationCoordinator, which properly usesshell=Trueto handle shell operators like&&andcd. The commands will execute correctly as written.Likely an incorrect or invalid review comment.
206-206: Missing timeout on URL download can cause indefinite hangs.
urllib.request.urlretrievedoes not support a timeout parameter and can hang indefinitely if the remote server is unresponsive or the connection stalls. This can cause the build process to freeze without any indication of progress.🔎 Proposed fix using urllib with timeout
- urllib.request.urlretrieve(url, archive_path) + # Use urlopen with timeout for better control + with urllib.request.urlopen(url, timeout=300) as response: + with open(archive_path, 'wb') as out_file: + shutil.copyfileobj(response, out_file)Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cortex/cli.pycortex/source_builder.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/source_builder.pycortex/cli.py
🧠 Learnings (1)
📚 Learning: 2025-12-11T12:03:24.071Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T12:03:24.071Z
Learning: Applies to **/*install*.py : Implement audit logging to ~/.cortex/history.db for all package operations
Applied to files:
cortex/cli.py
🧬 Code graph analysis (1)
cortex/source_builder.py (4)
cortex/branding.py (1)
cx_print(49-69)cortex/dependency_resolver.py (2)
DependencyResolver(40-386)is_package_installed(101-104)cortex/utils/commands.py (1)
run_command(223-305)cortex/kernel_features/llm_device.py (1)
open(158-159)
⏰ 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: Build Package
- GitHub Check: test (3.11)
- GitHub Check: test (3.10)
- GitHub Check: test (3.12)
🔇 Additional comments (6)
cortex/cli.py (1)
1627-1772: Excellent audit logging implementation!The
_install_from_sourcemethod implements comprehensive audit logging as requested in past review comments. The implementation properly:
- Records installation start with package name, version, source URL, and commands
- Updates history on success/failure for all execution modes (dry-run, non-execute, execute)
- Wraps all history operations in try-except blocks with appropriate logging
- Provides rollback information on successful installations
This follows the same pattern as the main
install()method and fully addresses the learnings requirement for audit logging to~/.cortex/history.db.Based on learnings, the implementation correctly adds audit logging for source-based installations.
cortex/source_builder.py (5)
92-111: Excellent cache directory initialization with robust error handling!The implementation properly addresses the past review comment about module-level side effects. The three-tier fallback strategy is well-designed:
- Try to create the preferred cache directory
- Fall back to temp directory if that fails
- Disable caching entirely if both fail
This ensures the module can be imported even in restrictive environments without failing.
201-270: Comprehensive SSRF protection implemented!The URL validation thoroughly prevents Server-Side Request Forgery attacks by:
- Restricting schemes to http/https only
- Resolving hostnames to both IPv4 and IPv6 addresses
- Rejecting loopback (127.0.0.1, ::1)
- Rejecting link-local addresses
- Rejecting private RFC1918 ranges (10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16)
- Rejecting cloud metadata service addresses (169.254.169.254)
This fully addresses the past review comment about validating source URLs to prevent SSRF attacks.
310-354: Excellent path traversal protection for archive extraction!The implementation properly prevents CVE-2007-4559 (tarfile) and zip slip attacks by:
- Manually filtering all archive members before extraction
- Rejecting paths with ".." components or absolute paths
- Resolving each member path and verifying it stays within the extract directory
- Applying consistent security checks to both tar.gz and zip formats
The manual filtering approach ensures compatibility across Python versions while maintaining security. The comment at line 314 properly explains why the
filter='data'parameter isn't used (Python 3.12+ only).
422-479: Smart command generation that works with validation constraints!The implementation cleverly addresses command validation constraints by:
- Using
bash configureinstead of./configureto satisfy ALLOWED_COMMAND_PREFIXES (line 450)- Returning tuples with working directories instead of using
cdcommands (line 473)- Properly quoting all arguments with
shlex.quoteto prevent injectionThis design successfully generates validated commands while maintaining full functionality. Great work addressing the past review comments about command validation failures!
1-711: Well-architected source building implementation with strong security.The overall design of this module is excellent:
Strengths:
- Clean separation of concerns (fetch, configure, build, install)
- Comprehensive security measures (SSRF protection, path traversal prevention)
- Robust error handling with graceful degradation (cache directory fallbacks)
- Smart workarounds for command validation constraints
- Proper use of type hints and dataclasses
- Good documentation throughout
Architecture highlights:
- Build caching reduces redundant work
- Automatic build system detection supports multiple ecosystems
- Dependency detection and installation automation
- Extensible design allows adding new build systems easily
The module successfully addresses all past review comments regarding security (SSRF, path traversal, cache initialization) and integrates well with the existing Cortex infrastructure.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @cortex/source_builder.py:
- Around line 277-466: The _fetch_from_url function currently leaves temporary
directories around on success; update build_from_source to remove the temporary
source dir after a successful build (use a finally block or post-build cleanup)
by detecting the temp pattern created by _fetch_from_url (e.g., path containing
"cortex-build-") and calling shutil.rmtree(..., ignore_errors=True) safely
inside build_from_source (ensure source_dir is not None and exists), or
alternatively add a cleanup method on BuildResult (e.g., BuildResult.cleanup)
that removes the temp build/source directory and call it at the end of
build_from_source to ensure no orphaned temp directories remain.
- Around line 132-149: _wrap the filesystem operations in _save_to_cache
(creating cache_path, writing metadata.json, touching installed) in a try/except
so IO errors don’t bubble out and break build_from_source; catch broad
OSError/IOError/Exception around the mkdir/json.dump/touch sequence, and on
exception log a warning including the cache_key and error details (use the class
logger or Python logging if available) and then return silently so the
successful build is not treated as failed due to cache save problems. Ensure you
reference cache_path, metadata.json and installed in the log so it’s clear which
cache artifact failed.
🧹 Nitpick comments (4)
cortex/source_builder.py (4)
174-177: Consider refining the Python package name heuristic.The check
if "python" in package_name.lower()is a simple substring match that could produce false positives (e.g., "mypython-tool" or "python-like"). While extra dependencies won't break builds, a more precise check would be cleaner.♻️ Suggested refinement
# Package-specific dependencies (common patterns) - if "python" in package_name.lower(): + # Match common Python package patterns: python, python3, cpython, pypy + pkg_lower = package_name.lower() + if pkg_lower in ("python", "python3", "cpython", "pypy") or \ + pkg_lower.startswith(("python-", "python3-", "cpython-")): required_deps.update(BUILD_DEPENDENCIES["python"])
468-486: Consider making source URL mappings configurable.The
_detect_source_locationmethod hardcodes URLs for only 3 packages. While this provides a reasonable starting point, making this configurable (e.g., loading from a JSON file or package database) would improve extensibility.💡 Suggested approach
Consider moving
common_urlsto a class attribute or external configuration file that can be extended without modifying code:# Could be loaded from ~/.cortex/source_urls.json DEFAULT_SOURCE_URLS = { "python": "https://www.python.org/ftp/python/{version}/Python-{version}.tgz", "nginx": "https://nginx.org/download/nginx-{version}.tar.gz", "redis": "https://download.redis.io/releases/redis-{version}.tar.gz", }
576-623: Consider returning a consistent type for better API clarity.The method returns
list[tuple[str, Path | None] | str], requiring callers to handle both tuples and strings withisinstancechecks. While functional, this is less clean than the consistent tuple return fromconfigure_build.♻️ Proposed fix for type consistency
- def build(self, source_dir: Path, config: BuildConfig) -> list[tuple[str, Path | None] | str]: + def build(self, source_dir: Path, config: BuildConfig) -> list[tuple[str, Path | None]]: """Build the package. Args: source_dir: Path to source code directory. config: Build configuration with options and settings. Returns: - List of build commands to execute. Can be: - - Tuples of (command, working_directory) for commands needing specific cwd - - Strings for commands that use source_dir as cwd + List of tuples (command, working_directory) where working_directory is + None to use source_dir as the default working directory. """ commands = [] if config.build_system == "autotools" or config.build_system == "make": make_cmd = "make" if config.make_args: # Sanitize make args to prevent injection safe_args = [shlex.quote(arg) for arg in config.make_args] make_cmd += " " + " ".join(safe_args) else: # Use parallel builds by default import multiprocessing jobs = multiprocessing.cpu_count() make_cmd += f" -j{jobs}" - commands.append(make_cmd) + commands.append((make_cmd, None)) # None means use source_dir elif config.build_system == "cmake": build_dir = source_dir / "build" make_cmd = "make" if config.make_args: # Sanitize make args safe_args = [shlex.quote(arg) for arg in config.make_args] make_cmd += " " + " ".join(safe_args) else: import multiprocessing jobs = multiprocessing.cpu_count() make_cmd += f" -j{jobs}" - # Use build_dir as working directory instead of cd command - # Return as tuple for consistency with configure_build commands.append((make_cmd, build_dir)) elif config.build_system == "python": - commands.append("python3 setup.py build") + commands.append(("python3 setup.py build", None)) return commandsThen simplify the caller in
build_from_source:build_commands = self.build(source_dir, config) for cmd_tuple in build_commands: - if isinstance(cmd_tuple, tuple): - cmd, work_dir = cmd_tuple - cwd = str(work_dir) if work_dir else str(source_dir) - else: - # Backward compatibility: if it's just a string, use source_dir - cmd = cmd_tuple - cwd = str(source_dir) + cmd, work_dir = cmd_tuple + cwd = str(work_dir) if work_dir else str(source_dir) result = run_command(cmd, cwd=cwd, timeout=3600)
719-736: Consider using shlex.quote for dependency installation command.While
missing_depscontains only hardcoded package names fromBUILD_DEPENDENCIES(not user input), usingshlex.quotewould follow defensive coding best practices.♻️ Suggested improvement
if missing_deps: cx_print(f" Installing: {', '.join(missing_deps)}", "info") - install_cmd = f"sudo apt-get install -y {' '.join(missing_deps)}" + safe_deps = [shlex.quote(dep) for dep in missing_deps] + install_cmd = f"sudo apt-get install -y {' '.join(safe_deps)}" result = run_command(install_cmd, timeout=600)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/spam-protection.ymlcortex/source_builder.py
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/spam-protection.yml
🧰 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/source_builder.py
🧬 Code graph analysis (1)
cortex/source_builder.py (3)
cortex/branding.py (1)
cx_print(49-69)cortex/dependency_resolver.py (2)
DependencyResolver(40-386)is_package_installed(101-104)cortex/utils/commands.py (1)
run_command(223-305)
🪛 GitHub Actions: CI
cortex/source_builder.py
[error] 342-342: TypeError in _fetch_from_url: '>' not supported between instances of 'MagicMock' and 'int' while comparing member.size to MAX_FILE_SIZE during tarball fetch.
[error] 466-466: RuntimeError: Failed to fetch source: '>' not supported between instances of 'MagicMock' and 'int' (raised while handling fetch failure).
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
- GitHub Check: test (3.10)
🔇 Additional comments (11)
cortex/source_builder.py (11)
1-55: LGTM! Module structure and constants are well-organized.The module header, imports, and constants follow PEP 8 guidelines. The security constants (MAX_EXTRACT_FILES, MAX_EXTRACT_SIZE, etc.) provide good protection against zip bomb attacks, and BUILD_DEPENDENCIES provides sensible defaults for common build systems.
58-84: LGTM! Dataclasses are well-structured.Both
BuildConfigandBuildResultdataclasses have appropriate type hints and clear field names. The use of modern Python| Nonesyntax for optional fields is consistent with the project's Python 3.10+ requirement.
98-116: LGTM! Excellent error handling for cache directory creation.The constructor properly defers cache directory creation to runtime with comprehensive error handling. The multi-level fallback (main cache → temp directory → disable caching) ensures the module remains usable even in restrictive environments.
186-205: LGTM! Clean delegation with comprehensive documentation.The
fetch_sourcemethod provides a clear public API with proper type hints and complete docstring.
207-276: LGTM! Comprehensive SSRF protection.The URL validation method provides robust protection against SSRF attacks by:
- Restricting to http/https schemes
- Resolving hostnames to IPs
- Rejecting loopback, link-local, private, and metadata service addresses
- Handling both IPv4 and IPv6
316-385: LGTM! Comprehensive security protections for tarfile extraction.The tarfile extraction implements multiple layers of security:
- Path traversal prevention (rejecting ".." and absolute paths)
- Resource limits (file count, file size, total size)
- Compression ratio checks (zip bomb protection)
- Path resolution validation
These protections address CVE-2007-4559 and SonarCloud S5042.
386-449: LGTM! Zipfile extraction has equivalent security protections.The zipfile extraction mirrors the tarfile security measures with appropriate checks for the zip format, including per-file compression ratio validation.
342-342: Pipeline failure is a test setup issue, not production code issue.The TypeError
'>' not supported between instances of 'MagicMock' and 'int'occurs because the test mocks don't properly configuremember.sizeas an integer. The production code is correct; the test setup needs to be fixed.In
tests/test_source_builder.py, ensure that when mocking tarfile members, thesizeattribute is set to an integer value:# Example fix in test mock_member = MagicMock() mock_member.size = 1024 # Set as int, not leave as MagicMock mock_member.name = "file.txt"
488-515: LGTM! Build system detection logic is sound.The detection order (autotools → cmake → make → python) is appropriate, with a sensible default to autotools for unrecognized projects.
517-574: LGTM! Configure command generation is secure and consistent.The method properly:
- Uses
bash configureinstead of./configureto comply with ALLOWED_COMMAND_PREFIXES- Quotes all arguments with
shlex.quoteto prevent injection- Returns tuples
(command, working_directory)instead of using shell operators likecd && ...- Handles different build systems appropriately
649-806: Note: This method is affected by issues in install_build.The
build_from_sourcemethod generates install commands viainstall_build(line 783). Sinceinstall_buildcurrently returns commands with shell operators that will fail validation, cmake builds will fail at installation time.The fix for
install_build(see previous comment) will require corresponding updates here to handle the new tuple return type.
aybanda
force-pushed
the
feature/package-build-from-source--101
branch
from
7c57b9d to
cdd5e0f
Compare
|
hey @Anshgrover23 can you check this |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cortex/cli.py (1)
638-647: Document the new parameters in the method docstring.The
install()method signature now includes three new parameters (from_source,source_url,version), but the method lacks a docstring documenting these parameters and their usage.As per coding guidelines, docstrings are required for all public APIs.
📝 Suggested docstring addition
def install( self, software: str, execute: bool = False, dry_run: bool = False, parallel: bool = False, from_source: bool = False, source_url: str | None = None, version: str | None = None, ): + """Install software packages. + + Args: + software: Software package(s) to install + execute: Execute the installation commands + dry_run: Show commands without executing + parallel: Enable parallel execution for multi-step installs + from_source: Build and install from source code + source_url: URL to source code (used with from_source) + version: Version to build (used with from_source) + + Returns: + int: Exit code (0 for success, 1 for failure) + """
🤖 Fix all issues with AI agents
In @cortex/cli.py:
- Around line 2016-2023: Add a comprehensive docstring to the
_install_from_source method: describe its purpose (installing a package from a
source URL), document all parameters (package_name: str, execute: bool, dry_run:
bool, source_url: str | None, version: str | None), explain return value (int
status/exit code), note side effects (invokes build/install, may execute
commands), and list raised exceptions or error cases and any special behavior
for dry_run vs execute; place the docstring immediately below the def
_install_from_source(...) signature in cortex/cli.py using the project’s
docstring style.
🧹 Nitpick comments (3)
cortex/cli.py (3)
2034-2038: Strengthen version parsing validation.The version parsing from
package@versionsyntax is simplistic and doesn't validate the format or handle edge cases.♻️ More robust version parsing
- # Parse version from package name if specified (e.g., python@3.12) - if "@" in package_name and not version: - parts = package_name.split("@") - package_name = parts[0] - version = parts[1] if len(parts) > 1 and parts[1] else None + # Parse version from package name if specified (e.g., python@3.12) + if "@" in package_name and not version: + parts = package_name.split("@", 1) # Split only on first @ + if len(parts) == 2 and parts[0] and parts[1]: + package_name = parts[0].strip() + version = parts[1].strip() + else: + self._print_error(f"Invalid package@version format: {package_name}") + return 1
2305-2319: Consider clarifying flag relationships in help text.The
--source-urland--pkg-versionflags are only meaningful when used with--from-source, but this relationship isn't explicit in the help text.♻️ Clearer help text
install_parser.add_argument( "--source-url", type=str, - help="URL to source code (for --from-source)", + help="URL to source code (requires --from-source)", ) install_parser.add_argument( "--pkg-version", type=str, - help="Version to build (for --from-source)", + help="Version to build (requires --from-source, or use package@version)", )
2044-2051: Consider using a separate metadata field instead of including non-command strings in the commands list.The commands list includes metadata strings like
"Source URL: {source_url}"and"Version: {version}"alongside actual installation command names. While the history system correctly handles these through snapshot-based rollback and ignores them during package extraction, this design is semantically unclear—the field is named "commands" but contains metadata. A separate metadata field would improve code clarity and make the audit trail more maintainable.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/codeql.ymlcortex/cli.pytests/test_cli.py
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/codeql.yml
- tests/test_cli.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.py
🧠 Learnings (1)
📚 Learning: 2025-12-11T12:03:24.071Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T12:03:24.071Z
Learning: Applies to **/*install*.py : Implement audit logging to ~/.cortex/history.db for all package operations
Applied to 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). (4)
- GitHub Check: Test (Python 3.11)
- GitHub Check: test (3.12)
- GitHub Check: test (3.10)
- GitHub Check: test (3.11)
🔇 Additional comments (5)
cortex/cli.py (5)
10-11: LGTM: Logger initialization follows best practices.The module-level logger is properly initialized and used appropriately for non-critical warnings in the source build flow.
681-683: LGTM: Source build routing is well-placed.The early routing to
_install_from_source()appropriately bypasses the LLM-based flow and API key requirement when building from source, while still benefiting from input validation.
2027-2030: LGTM: Audit logging properly implemented.The method correctly initializes
InstallationHistoryand tracks installation operations throughout the source build flow, aligning with the requirement to audit log all package operations to~/.cortex/history.db.Based on learnings, this implements the required audit logging pattern.
2062-2084: LGTM: Comprehensive error handling and history tracking.The error handling properly:
- Catches build failures
- Updates history records with error messages
- Uses logger.warning for non-critical failures
- Returns appropriate exit codes
2686-2688: LGTM: Safe parameter forwarding.The use of
getattrwith appropriate defaults provides defensive programming against missing attributes while cleanly forwarding the new parameters to theinstall()method.
| def _install_from_source( | ||
| self, | ||
| package_name: str, | ||
| execute: bool, | ||
| dry_run: bool, | ||
| source_url: str | None, | ||
| version: str | None, | ||
| ) -> int: |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add comprehensive docstring for this method.
The _install_from_source() method lacks a docstring. Per coding guidelines, docstrings are required for all public APIs, and this method is a significant entry point for source-build functionality.
📝 Suggested docstring
def _install_from_source(
self,
package_name: str,
execute: bool,
dry_run: bool,
source_url: str | None,
version: str | None,
) -> int:
+ """Build and install a package from source.
+
+ Handles the complete source build workflow including dependency detection,
+ build system detection, compilation, and installation. Integrates with
+ InstallationHistory for audit logging.
+
+ Args:
+ package_name: Name of the package to build (supports package@version syntax)
+ execute: Execute installation commands after building
+ dry_run: Show commands without executing
+ source_url: Optional URL to source code (tarball, GitHub, etc.)
+ version: Optional version to build (can also be specified via package@version)
+
+ Returns:
+ int: Exit code (0 for success, 1 for failure)
+ """📝 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.
| def _install_from_source( | |
| self, | |
| package_name: str, | |
| execute: bool, | |
| dry_run: bool, | |
| source_url: str | None, | |
| version: str | None, | |
| ) -> int: | |
| def _install_from_source( | |
| self, | |
| package_name: str, | |
| execute: bool, | |
| dry_run: bool, | |
| source_url: str | None, | |
| version: str | None, | |
| ) -> int: | |
| """Build and install a package from source. | |
| Handles the complete source build workflow including dependency detection, | |
| build system detection, compilation, and installation. Integrates with | |
| InstallationHistory for audit logging. | |
| Args: | |
| package_name: Name of the package to build (supports package@version syntax) | |
| execute: Execute installation commands after building | |
| dry_run: Show commands without executing | |
| source_url: Optional URL to source code (tarball, GitHub, etc.) | |
| version: Optional version to build (can also be specified via package@version) | |
| Returns: | |
| int: Exit code (0 for success, 1 for failure) | |
| """ |
🤖 Prompt for AI Agents
In @cortex/cli.py around lines 2016 - 2023, Add a comprehensive docstring to the
_install_from_source method: describe its purpose (installing a package from a
source URL), document all parameters (package_name: str, execute: bool, dry_run:
bool, source_url: str | None, version: str | None), explain return value (int
status/exit code), note side effects (invokes build/install, may execute
commands), and list raised exceptions or error cases and any special behavior
for dry_run vs execute; place the docstring immediately below the def
_install_from_source(...) signature in cortex/cli.py using the project’s
docstring style.
✅ Addressed in commit bd4027b
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
cortex/cli.py (3)
2024-2072: Condense the docstring significantly.This 48-line docstring is excessively verbose and violates typical Python/Google style guidelines. While comprehensive documentation is valuable, docstrings should be concise. Consider:
- Reducing the Args/Returns/Side Effects sections to essential information
- Moving detailed usage examples and special behaviors to module-level docs or SOURCE_BUILD.md
- Keeping the docstring under 20-25 lines
Example condensed version
- def _install_from_source( - self, - package_name: str, - execute: bool, - dry_run: bool, - source_url: str | None, - version: str | None, - ) -> int: - """Install a package from a source URL by building and optionally installing it. - - This method handles the complete workflow for installing packages from source code: - parsing version information, building the package, and optionally executing - installation commands. It supports dry-run mode for previewing operations and - records all activities in the installation history for audit purposes. - - Args: - package_name: Name of the package to install. If version is specified - using "@" syntax (e.g., "python@3.12"), it will be parsed automatically - if version parameter is None. - execute: If True, executes the installation commands after building. - If False, only builds the package and displays commands without executing. - dry_run: If True, performs a dry run showing what commands would be executed - without actually building or installing. Takes precedence over execute. - source_url: Optional URL to the source code repository or tarball. - If None, the SourceBuilder will attempt to locate the source automatically. - version: Optional version string to build. If None and package_name contains - "@", the version will be extracted from package_name. - - Returns: - int: Exit status code. Returns 0 on success (build/install completed or - dry-run completed), 1 on failure (build failed or installation failed). - - Side Effects: - - Invokes SourceBuilder.build_from_source() to build the package - - May execute installation commands via InstallationCoordinator if execute=True - - Records installation start, progress, and completion in InstallationHistory - - Prints status messages and progress to console - - May use cached builds if available - - Raises: - No exceptions are raised directly, but underlying operations may fail: - - SourceBuilder.build_from_source() failures are caught and returned as status 1 - - InstallationCoordinator.execute() failures are caught and returned as status 1 - - InstallationHistory exceptions are caught and logged as warnings - - Special Behavior: - - dry_run=True: Shows build/install commands without executing any operations. - Returns 0 after displaying commands. Installation history is still recorded. - - execute=False, dry_run=False: Builds the package and displays install commands - but does not execute them. Returns 0. User is prompted to run with --execute. - - execute=True, dry_run=False: Builds the package and executes all installation - commands. Returns 0 on success, 1 on failure. - - Version parsing: If package_name contains "@" (e.g., "python@3.12") and version - is None, the version is automatically extracted and package_name is updated. - - Caching: Uses cached builds when available, printing a notification if cache - is used. - """ + def _install_from_source( + self, + package_name: str, + execute: bool, + dry_run: bool, + source_url: str | None, + version: str | None, + ) -> int: + """Build and install a package from source code. + + Supports version pinning (package@version syntax), source URLs, dry-run mode, + and build caching. Records all operations to installation history for audit/rollback. + + Args: + package_name: Package name, optionally with "@version" suffix. + execute: Execute install commands after building. + dry_run: Preview commands without building or installing. + source_url: Optional source repository or tarball URL. + version: Optional version to build (overridden by package_name@version). + + Returns: + 0 on success, 1 on failure. + """
2166-2180: Extract duplicated coordinator setup to reduce code duplication.The progress callback (lines 2166-2172) and InstallationCoordinator setup (lines 2174-2180) are duplicated from the main
install()method (lines 725-732, 823-829). This violates the DRY principle and makes maintenance harder.Consider extracting this into a helper method like
_execute_with_coordinator(commands, descriptions, package_name)that bothinstall()and_install_from_source()can call.Proposed helper method
def _execute_with_coordinator( self, commands: list[str], descriptions: list[str], timeout: int = 600 ) -> tuple[bool, str | None]: """Execute commands using InstallationCoordinator with progress reporting. Returns: Tuple of (success: bool, error_message: str | None) """ def progress_callback(current: int, total: int, step: InstallationStep) -> None: status_emoji = "⏳" if step.status == StepStatus.SUCCESS: status_emoji = "✅" elif step.status == StepStatus.FAILED: status_emoji = "❌" console.print(f"[{current}/{total}] {status_emoji} {step.description}") coordinator = InstallationCoordinator( commands=commands, descriptions=descriptions, timeout=timeout, stop_on_error=True, progress_callback=progress_callback, ) result = coordinator.execute() return result.success, result.error_messageThen use it in both methods.
2176-2176: Provide distinct descriptions for each install command.All install commands receive the same description
f"Install {package_name}", making progress output non-descriptive when multiple commands are executed. Consider using more specific descriptions like:
f"Install dependencies for {package_name}"f"Install {package_name} binary"- Or derive descriptions from the command itself
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cortex/cli.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.py
🧠 Learnings (1)
📚 Learning: 2025-12-11T12:03:24.071Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T12:03:24.071Z
Learning: Applies to **/*install*.py : Implement audit logging to ~/.cortex/history.db for all package operations
Applied to 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). (4)
- GitHub Check: Test (Python 3.11)
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
- GitHub Check: test (3.10)
🔇 Additional comments (4)
cortex/cli.py (4)
638-647: LGTM: Clean signature extension.The new parameters (
from_source,source_url,version) are properly typed with sensible defaults, maintaining backward compatibility.
681-683: LGTM: Clean routing logic.The early return when
from_source=Trueproperly delegates to the new source-build path and prevents mixing with the LLM-based installation flow.
2353-2367: LGTM: CLI arguments are well-defined.The new
--from-source,--source-url, and--pkg-versionarguments are properly configured with clear help text and appropriate types.
2734-2736: LGTM: Safe argument forwarding.Using
getattrwith default values provides defensive handling and ensures the code won't fail if arguments are missing, though argparse should always set these attributes.
Anshgrover23
left a comment
Anshgrover23
left a comment
There was a problem hiding this comment.
@aybanda Kindly address coderabbtAI comments.
There was a problem hiding this comment.
No need to change this file.
There was a problem hiding this comment.
Why this one changed ?
| def _install_from_source( | ||
| self, | ||
| package_name: str, | ||
| execute: bool, | ||
| dry_run: bool, | ||
| source_url: str | None, | ||
| version: str | None, | ||
| ) -> int: |
Anshgrover23
left a comment
Anshgrover23
left a comment
There was a problem hiding this comment.
@aybanda Are u working on this one, If then kindly address coderabbitai comments.
aybanda
force-pushed
the
feature/package-build-from-source--101
branch
from
14e2111 to
ac5cd85
Compare
There was a problem hiding this comment.
Pull request overview
This PR implements a comprehensive source build feature for Cortex Linux that enables building and installing packages from source code when pre-built binaries are unavailable. The implementation includes automatic build system detection, dependency management, caching, and CLI integration.
Changes:
- Added
SourceBuilderclass with support for autotools, cmake, make, and python build systems - Integrated source build workflow into CLI with
--from-source,--source-url, and--pkg-versionflags - Implemented SSRF protection for URL validation and path traversal protection for archive extraction
- Added comprehensive test suite with 80%+ coverage for source builder module
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| cortex/source_builder.py | Core module implementing source fetching, build system detection, dependency resolution, building, and caching |
| cortex/cli.py | CLI integration adding _install_from_source method and new command-line arguments |
| cortex/utils/commands.py | Added bash and sh to allowed command prefixes for build scripts |
| tests/test_source_builder.py | Comprehensive unit tests for SourceBuilder class and related functionality |
| tests/test_cli.py | Updated CLI tests to include new install parameters |
| tests/test_cli_extended.py | Extended CLI tests updated for new install signature |
| docs/SOURCE_BUILD.md | Complete documentation guide for source building feature |
| pyproject.toml | Excluded docs directory from linting |
| .github/workflows/spam-protection.yml | Minor formatting change to comment template |
| .github/workflows/codeql.yml | Added autobuild step for CodeQL analysis |
Comments suppressed due to low confidence (5)
cortex/cli.py:2096
- The install_id variable is assigned None twice (lines 2095 and 2096) which is redundant. Remove the duplicate assignment on line 2096.
confirm = input(f"⚠️ Clear ALL environment variables for '{app}'? (y/n): ")
if confirm.lower() != "y":
cortex/cli.py:2212
- The new _install_from_source method in CLI lacks test coverage. No tests verify the integration between CLI arguments, SourceBuilder, and InstallationCoordinator for the source build workflow. Add tests covering the happy path, error handling, dry-run mode, and execute mode.
def _env_export(self, env_mgr: EnvironmentManager, args: argparse.Namespace) -> int:
"""Export environment variables to .env format."""
app = args.app
include_encrypted = getattr(args, "include_encrypted", False)
output_file = getattr(args, "output", None)
content = env_mgr.export_env(app, include_encrypted=include_encrypted)
if not content:
cx_print(f"No environment variables to export for '{app}'", "info")
return 0
if output_file:
try:
with open(output_file, "w", encoding="utf-8") as f:
f.write(content)
cx_print(f"✓ Exported to {output_file}", "success")
except OSError as e:
self._print_error(f"Failed to write file: {e}")
return 1
else:
# Print to stdout
print(content, end="")
return 0
def _env_import(self, env_mgr: EnvironmentManager, args: argparse.Namespace) -> int:
"""Import environment variables from .env format."""
import sys
app = args.app
input_file = getattr(args, "file", None)
encrypt_keys = getattr(args, "encrypt_keys", None)
try:
if input_file:
with open(input_file, encoding="utf-8") as f:
content = f.read()
elif not sys.stdin.isatty():
content = sys.stdin.read()
else:
self._print_error("No input file specified and stdin is empty")
cx_print("Usage: cortex env import <app> <file>", "info")
cx_print(" or: cat .env | cortex env import <app>", "info")
return 1
# Parse encrypt-keys argument
encrypt_list = []
if encrypt_keys:
encrypt_list = [k.strip() for k in encrypt_keys.split(",")]
count, errors = env_mgr.import_env(app, content, encrypt_keys=encrypt_list)
if errors:
for err in errors:
cx_print(f" ⚠ {err}", "warning")
if count > 0:
cx_print(f"✓ Imported {count} variable(s) to '{app}'", "success")
else:
cx_print("No variables imported", "info")
# Return success (0) even with partial errors - some vars imported successfully
return 0
except FileNotFoundError:
self._print_error(f"File not found: {input_file}")
return 1
except OSError as e:
self._print_error(f"Failed to read file: {e}")
return 1
def _env_clear(self, env_mgr: EnvironmentManager, args: argparse.Namespace) -> int:
"""Clear all environment variables for an app."""
app = args.app
force = getattr(args, "force", False)
# Confirm unless --force is used
if not force:
confirm = input(f"⚠️ Clear ALL environment variables for '{app}'? (y/n): ")
if confirm.lower() != "y":
cx_print("Operation cancelled", "info")
return 0
if env_mgr.clear_app(app):
cx_print(f"✓ Cleared all variables for '{app}'", "success")
else:
cx_print(f"No environment data found for '{app}'", "info")
return 0
def _env_template(self, env_mgr: EnvironmentManager, args: argparse.Namespace) -> int:
"""Handle template subcommands."""
template_action = getattr(args, "template_action", None)
if template_action == "list":
return self._env_template_list(env_mgr)
elif template_action == "show":
return self._env_template_show(env_mgr, args)
elif template_action == "apply":
return self._env_template_apply(env_mgr, args)
else:
self._print_error(
"Please specify: template list, template show <name>, "
"or template apply <name> <app>"
)
return 1
def _env_template_list(self, env_mgr: EnvironmentManager) -> int:
"""List available templates."""
templates = env_mgr.list_templates()
cx_header("Available Environment Templates")
for template in sorted(templates, key=lambda t: t.name):
console.print(f" [green]{template.name}[/green]")
console.print(f" {template.description}")
console.print(f" [dim]{len(template.variables)} variables[/dim]")
console.print()
cx_print("Use 'cortex env template show <name>' for details", "info")
return 0
def _env_template_show(self, env_mgr: EnvironmentManager, args: argparse.Namespace) -> int:
"""Show template details."""
template_name = args.template_name
template = env_mgr.get_template(template_name)
if not template:
self._print_error(f"Template '{template_name}' not found")
return 1
cx_header(f"Template: {template.name}")
console.print(f" {template.description}")
console.print()
console.print("[bold]Variables:[/bold]")
for var in template.variables:
req = "[red]*[/red]" if var.required else " "
default = f" = {var.default}" if var.default else ""
console.print(f" {req} [cyan]{var.name}[/cyan] ({var.var_type}){default}")
if var.description:
console.print(f" [dim]{var.description}[/dim]")
console.print()
console.print("[dim]* = required[/dim]")
return 0
def _env_template_apply(self, env_mgr: EnvironmentManager, args: argparse.Namespace) -> int:
"""Apply a template to an app."""
template_name = args.template_name
app = args.app
# Parse key=value pairs from args
values = {}
value_args = getattr(args, "values", []) or []
for val in value_args:
if "=" in val:
k, v = val.split("=", 1)
values[k] = v
# Parse encrypt keys
encrypt_keys = []
encrypt_arg = getattr(args, "encrypt_keys", None)
if encrypt_arg:
encrypt_keys = [k.strip() for k in encrypt_arg.split(",")]
result = env_mgr.apply_template(
template_name=template_name,
app=app,
values=values,
encrypt_keys=encrypt_keys,
)
if result.valid:
cx_print(f"✓ Applied template '{template_name}' to '{app}'", "success")
return 0
else:
self._print_error(f"Failed to apply template '{template_name}'")
for err in result.errors:
console.print(f" [red]✗[/red] {err}")
return 1
def _env_list_apps(self, env_mgr: EnvironmentManager, args: argparse.Namespace) -> int:
"""List all apps with stored environments."""
apps = env_mgr.list_apps()
if not apps:
cx_print("No applications with stored environments", "info")
return 0
cx_header("Applications with Environments")
for app in apps:
var_count = len(env_mgr.list_variables(app))
console.print(f" [green]{app}[/green] [dim]({var_count} variables)[/dim]")
return 0
cortex/cli.py:2370
- The argument name
--pkg-versionis inconsistent with the method parameter nameversionand usage throughout the codebase. Consider renaming to--versionfor better consistency and user experience, or clearly document why the different naming was chosen.
else:
self._print_error(f"Unknown path action: {path_action}")
return 1
def _env_path_list(self, analyzer: "ShellEnvironmentAnalyzer", args: argparse.Namespace) -> int:
cortex/cli.py:2072
- The docstring states that dry_run performs a dry run without building, but the actual implementation (line 2111) calls builder.build_from_source() which does perform the full build. The dry run only skips execution of install commands. Update the docstring to accurately reflect that dry_run builds the package but doesn't execute install commands.
if not content:
cx_print(f"No environment variables to export for '{app}'", "info")
return 0
if output_file:
try:
with open(output_file, "w", encoding="utf-8") as f:
f.write(content)
cx_print(f"✓ Exported to {output_file}", "success")
except OSError as e:
self._print_error(f"Failed to write file: {e}")
return 1
else:
# Print to stdout
print(content, end="")
return 0
def _env_import(self, env_mgr: EnvironmentManager, args: argparse.Namespace) -> int:
"""Import environment variables from .env format."""
import sys
app = args.app
input_file = getattr(args, "file", None)
encrypt_keys = getattr(args, "encrypt_keys", None)
try:
if input_file:
with open(input_file, encoding="utf-8") as f:
content = f.read()
elif not sys.stdin.isatty():
content = sys.stdin.read()
else:
self._print_error("No input file specified and stdin is empty")
cx_print("Usage: cortex env import <app> <file>", "info")
cx_print(" or: cat .env | cortex env import <app>", "info")
return 1
# Parse encrypt-keys argument
encrypt_list = []
if encrypt_keys:
encrypt_list = [k.strip() for k in encrypt_keys.split(",")]
count, errors = env_mgr.import_env(app, content, encrypt_keys=encrypt_list)
if errors:
for err in errors:
cx_print(f" ⚠ {err}", "warning")
cortex/cli.py:2137
- Variable commands is not used.
return 0
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Fallback: try configure.ac (needs autogen) | ||
| configure_script = source_dir / "configure.ac" | ||
| if configure_script.exists(): | ||
| # Would need autogen.sh first, but for now use configure | ||
| configure_script = source_dir / "configure" |
There was a problem hiding this comment.
When the configure script doesn't exist for autotools builds (lines 453-458), the code attempts to fall back to configure.ac but then still uses 'configure' as the script path. This will fail if configure doesn't exist. The logic should either run autogen.sh/autoconf to generate configure from configure.ac, or return an error if configure is not found. As written, it will try to execute a non-existent configure script.
| # Fallback: try configure.ac (needs autogen) | |
| configure_script = source_dir / "configure.ac" | |
| if configure_script.exists(): | |
| # Would need autogen.sh first, but for now use configure | |
| configure_script = source_dir / "configure" | |
| # Fallback: try configure.ac and optionally autogen.sh to generate configure | |
| configure_ac = source_dir / "configure.ac" | |
| if not configure_ac.exists(): | |
| raise FileNotFoundError( | |
| f"No autotools configure script found in {source_dir}: " | |
| f"neither 'configure' nor 'configure.ac' exist." | |
| ) | |
| autogen_script = source_dir / "autogen.sh" | |
| if autogen_script.exists(): | |
| # First run autogen.sh to generate the configure script | |
| autogen_cmd = f"bash {shlex.quote(str(autogen_script))}" | |
| commands.append((autogen_cmd, source_dir)) | |
| # After autogen.sh, we expect a 'configure' script to be present | |
| configure_script = source_dir / "configure" | |
| else: | |
| raise FileNotFoundError( | |
| f"'configure' script not found in {source_dir}, and no 'autogen.sh' " | |
| f"script is available to generate it from 'configure.ac'." | |
| ) |
| # Reject private RFC1918 addresses (10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16) | ||
| if ip.is_private: | ||
| raise ValueError(f"Private address {ip} is not allowed.") |
There was a problem hiding this comment.
The SSRF validation rejects all private IP addresses, but this may be too restrictive for legitimate enterprise use cases where source code repositories are hosted on internal networks. Consider adding a configuration option to allow private IPs when needed, or at minimum document this limitation clearly.
| def _detect_source_location(self, package_name: str, version: str | None) -> Path: | ||
| """Detect source location from package name.""" | ||
| # Try common patterns | ||
| common_urls = { | ||
| "python": ( | ||
| f"https://www.python.org/ftp/python/{version or '3.12.0'}/" | ||
| f"Python-{version or '3.12.0'}.tgz" | ||
| ), | ||
| "nginx": "https://nginx.org/download/nginx-1.24.0.tar.gz", | ||
| "redis": (f"https://download.redis.io/releases/redis-{version or '7.0'}.tar.gz"), | ||
| } | ||
|
|
||
| if package_name.lower() in common_urls: | ||
| return self._fetch_from_url(common_urls[package_name.lower()], package_name, version) | ||
|
|
||
| raise ValueError( | ||
| f"Could not detect source location for {package_name}. " | ||
| "Please provide --source-url or configure source location." | ||
| ) |
There was a problem hiding this comment.
The _detect_source_location method does not validate the constructed URLs before passing them to _fetch_from_url. While _fetch_from_url does validate, the URLs in common_urls are hardcoded and should be validated at initialization or documented as trusted. Additionally, when version is None, default versions are used ('3.12.0', '7.0') but there's no validation that these versions actually exist, which could lead to download failures with unclear error messages.
| import multiprocessing | ||
|
|
||
| jobs = multiprocessing.cpu_count() | ||
| make_cmd += f" -j{jobs}" | ||
| commands.append(make_cmd) | ||
|
|
||
| elif config.build_system == "cmake": | ||
| build_dir = source_dir / "build" | ||
| make_cmd = "make" | ||
| if config.make_args: | ||
| # Sanitize make args | ||
| safe_args = [shlex.quote(arg) for arg in config.make_args] | ||
| make_cmd += " " + " ".join(safe_args) | ||
| else: | ||
| import multiprocessing | ||
|
|
||
| jobs = multiprocessing.cpu_count() | ||
| make_cmd += f" -j{jobs}" |
There was a problem hiding this comment.
The multiprocessing module is imported inside the build method (lines 514, 528), which is inefficient as it's imported on every build call. Move the import to the top of the file or module level to follow Python best practices and improve performance.
| logger.exception(f"Build failed for {package_name}") | ||
| # Clean up temporary directory on error | ||
| try: | ||
| source_path = source_dir.resolve() if source_dir else None |
There was a problem hiding this comment.
The cleanup logic attempts to access source_dir which may not be defined if an exception occurs before the fetch_source call on line 617. The variable should be initialized to None before the try block to avoid NameError in the exception handler.
| def _get_cache_key(self, package_name: str, version: str | None, source_url: str) -> str: | ||
| """Generate a cache key for a build.""" | ||
| key_data = f"{package_name}:{version}:{source_url}" | ||
| return hashlib.sha256(key_data.encode()).hexdigest()[:16] | ||
|
|
||
| def _check_cache(self, cache_key: str) -> Path | None: | ||
| """Check if a build is cached.""" | ||
| if self.cache_dir is None: | ||
| return None | ||
| cache_path = self.cache_dir / cache_key | ||
| if cache_path.exists() and (cache_path / "installed").exists(): | ||
| return cache_path | ||
| return None | ||
|
|
||
| def _save_to_cache(self, cache_key: str, build_dir: Path, install_commands: list[str]) -> None: | ||
| """Save build artifacts to cache. | ||
| Args: | ||
| cache_key: Cache key for the build. | ||
| build_dir: Path to build directory. | ||
| install_commands: List of install commands. | ||
| """ | ||
| if self.cache_dir is None: | ||
| return # Caching disabled | ||
|
|
||
| try: | ||
| cache_path = self.cache_dir / cache_key | ||
| cache_path.mkdir(parents=True, exist_ok=True) | ||
|
|
||
| # Save metadata | ||
| metadata = { | ||
| "build_dir": str(build_dir), | ||
| "install_commands": install_commands, | ||
| "timestamp": str(Path(build_dir).stat().st_mtime), | ||
| } | ||
| with open(cache_path / "metadata.json", "w") as f: | ||
| json.dump(metadata, f, indent=2) | ||
|
|
||
| # Mark as installed | ||
| (cache_path / "installed").touch() | ||
| except Exception as e: | ||
| # Log error but don't fail the entire operation | ||
| logger.warning(f"Failed to save cache for {cache_key}: {e}") |
There was a problem hiding this comment.
There are no tests for the caching functionality (_get_cache_key, _check_cache, _save_to_cache). Add tests to verify cache key generation, cache hit/miss scenarios, cache metadata persistence, and cache retrieval.
|
|
||
| from __future__ import annotations | ||
|
|
||
| import os |
There was a problem hiding this comment.
Import of 'os' is not used.
| import os |
| from cortex.source_builder import ( | ||
| BUILD_DEPENDENCIES, | ||
| BuildConfig, | ||
| BuildResult, |
There was a problem hiding this comment.
Import of 'BuildResult' is not used.
| BuildResult, |
| # Try IPv4 first | ||
| ipv4 = socket.gethostbyname(hostname) | ||
| ip_addresses.append(ipaddress.IPv4Address(ipv4)) | ||
| except (socket.gaierror, ValueError): |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| for info in addrinfo: | ||
| ipv6 = info[4][0] | ||
| ip_addresses.append(ipaddress.IPv6Address(ipv6)) | ||
| except (socket.gaierror, ValueError, OSError): |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@cortex/cli.py`:
- Around line 2935-2940: The current flow calls builder.build_from_source(...)
before checking the --dry-run flag, causing a full build even for dry runs;
short-circuit this by moving the dry-run check earlier or add a dry-run/plan
mode to SourceBuilder and pass it through. Specifically, before invoking
builder.build_from_source, inspect the dry_run flag (the CLI option handling the
--dry-run) and if set, either skip calling builder.build_from_source and return
a planned result, or extend SourceBuilder (and its build_from_source signature)
with a dry_run/plan parameter and thread that through so build_from_source
performs a no-op/plan-only operation when dry_run is true. Ensure all callers of
builder.build_from_source in this command and any helper that constructs
SourceBuilder forward the new dry_run flag to avoid unintended real builds
during dry runs.
- Around line 2921-2930: The history entry is being created too early with an
empty commands list and only when execute or dry_run is true, which misses
logging for the build-only path; change the flow so record_installation
(InstallationType.INSTALL) is invoked after the build completes using the real
commands (use result.install_commands) and ensure it is called regardless of
execute/dry_run (or alternatively extend InstallationHistory/record_installation
to accept a follow-up update of commands and call that with
result.install_commands); update references around package_name,
record_installation, result.install_commands, execute, and dry_run to guarantee
every operation (including build-only) writes the full commands into
~/.cortex/history.db.
- Around line 2999-3005: Wrap all untrusted build/install invocations with the
Firejail sandbox by using the sandbox executor utilities: modify calls to
SourceBuilder.build_from_source to invoke the sandboxed execution path (use the
sandbox executor class in sandbox_executor.py, e.g., FirejailExecutor or
SandboxExecutor) so the actual build commands run through Firejail, and change
InstallationCoordinator instantiation to pass/use the sandbox executor for
result.install_commands (or wrap each command string with the sandbox executor
invocation) and any dependency install steps; ensure you wire the sandbox
executor into the progress_callback/execution pipeline used by
InstallationCoordinator so all coordinator-executed commands are run via the
Firejail wrapper rather than directly on the host.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cortex/cli.pypyproject.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide for Python code
Include type hints in Python code
Add docstrings for all public APIs in Python code
Use dry-run mode by default for all installation operations
Do not use silent sudo - require explicit user confirmation for privilege escalation
Implement Firejail sandboxing for execution of untrusted code
Log all operations to ~/.cortex/history.db for audit purposes
Files:
cortex/cli.py
🧠 Learnings (1)
📚 Learning: 2026-01-12T20:51:13.828Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-12T20:51:13.828Z
Learning: Applies to **/*.py : Log all operations to ~/.cortex/history.db for audit purposes
Applied to files:
cortex/cli.py
🧬 Code graph analysis (1)
cortex/cli.py (2)
cortex/source_builder.py (1)
build_from_source(566-752)cortex/installation_history.py (4)
record_installation(259-317)InstallationType(27-35)update_installation(319-368)InstallationStatus(38-44)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test (3.11)
- GitHub Check: test (3.10)
- GitHub Check: test (3.12)
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
aybanda
force-pushed
the
feature/package-build-from-source--101
branch
from
432c118 to
863cdf8
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@cortex/cli.py`:
- Around line 860-862: The current branch returns into
_install_from_source(software, execute, dry_run, source_url, version) when
from_source is True but _install_from_source still performs downloads,
dependency installation and compilation even if execute is False; update
_install_from_source (referenced here) to honor the execute/dry_run flags: when
execute is False (or dry_run True) it must not perform any real side-effecting
steps (download, install build deps, run compile commands) and instead only log
or simulate the steps and return a successful dry-run result; ensure all
internal calls that spawn subprocesses or write files check the execute flag
first and that the top-level call from the from_source branch passes the correct
dry_run/execute flags through to enforce dry-run behavior.
- Around line 3058-3062: The Firejail branch assigns install_result using
InstallationResult but the symbol isn’t imported, causing a NameError; add a
proper import for InstallationResult at the top of the module (import the
InstallationResult class from the module where it is defined) so the assignment
to install_result in the Firejail path succeeds without error and ensure the
import is grouped with other CLI imports; reference symbols: InstallationResult
and install_result.
In `@cortex/source_builder.py`:
- Around line 657-665: The code auto-runs privileged installs using install_cmd
and run_command when missing_deps is non-empty (detected by
detect_build_dependencies) without prompting; change this flow to request
explicit user confirmation via cx_print and an interactive prompt (or a
passed-in flag) before constructing/running install_cmd, aborting or returning
an error if the user declines, and ensure the prompt mentions the exact packages
(missing_deps) and the use of sudo so callers of
detect_build_dependencies/install_cmd/run_command can opt out or provide
non‑privileged alternatives.
- Around line 300-321: The code always saves downloads to a hardcoded
archive_path = temp_dir / "source.tar.gz", which breaks handling of .zip
archives; change the download/save logic in the block around archive_path,
urllib.request.urlretrieve(...) and subsequent extraction code so the downloaded
filename/extension matches the URL (e.g., derive archive_path from URL path or
use tempfile with proper suffix), re-run self._validate_url as before, and then
branch extraction based on the actual saved file suffix (reference symbols:
archive_path, urllib.request.urlretrieve, extract_dir, and the extraction
conditional that checks archive_path.suffix / suffixes). Ensure zip downloads
are saved with .zip and extracted with the zip branch while tar.gz keeps the tar
extraction flow.
♻️ Duplicate comments (6)
cortex/source_builder.py (6)
126-154: Cache stores metadata only; cached builds won’t be usable.
_save_to_cachewrites metadata but does not persist build artifacts, while later cache hits return install commands pointing to now‑deleted build dirs. This makes caching ineffective.
449-459: Autotools fallback still invokes missingconfigure.
If onlyconfigure.acexists, the code still points toconfigurewithout generating it.
553-563: Install commands for autotools/python execute in the wrong directory.
sudo make installandsudo python3 setup.py installdepend on the source/build directory, but the CLI executes commands from the current working directory.🐛 Suggested fix
- if config.build_system == "autotools" or config.build_system == "make": - commands.append("sudo make install") + if config.build_system == "autotools" or config.build_system == "make": + commands.append(f"sudo make -C {shlex.quote(str(source_dir))} install") ... - elif config.build_system == "python": - commands.append("sudo python3 setup.py install") + elif config.build_system == "python": + commands.append( + f"sudo python3 {shlex.quote(str(source_dir / 'setup.py'))} install" + )
597-640:source_dircan be undefined in the exception handler.
If an exception occurs beforefetch_source, theexceptblock referencessource_dirand can raiseUnboundLocalError.🐛 Suggested fix
- try: + source_dir: Path | None = None + try: # Handle dry-run mode: skip actual build and return planned result if dry_run:
677-710: Build/configure steps run outside Firejail.
Untrusted build commands execute viarun_commandwithout the sandbox. Please route these throughSandboxExecutor(or allow an executor to be injected) to comply with the sandboxing requirement.As per coding guidelines, untrusted code must be sandboxed.
728-742: Build artifacts are deleted before installation executes.
build_from_sourceremoves the temp directory before_install_from_sourceruns install commands, somake install/setup.py installwill fail and cached paths become invalid.🐛 Suggested fix
- # Clean up temporary directories after successful build - # Find and remove the parent temp directory if it's in /tmp - try: - source_path = source_dir.resolve() - if "/tmp" in str(source_path) or "/var/tmp" in str(source_path): - # Find the top-level temp directory created by cortex - temp_parent = source_path - while temp_parent.parent != temp_parent: # Stop at root - if "cortex-build-" in temp_parent.name: - # This is the main temp directory created by mkdtemp - shutil.rmtree(temp_parent, ignore_errors=True) - break - temp_parent = temp_parent.parent - except Exception as e: - logger.warning(f"Failed to clean up temporary directory: {e}") + # Defer cleanup until after install commands execute
🧹 Nitpick comments (1)
cortex/cli.py (1)
817-826: Add return type + docstring forinstall().
Public API lacks a return type annotation and docstring despite new parameters. This improves contract clarity and aligns with project standards.♻️ Suggested update
- def install( + def install( self, software: str, execute: bool = False, dry_run: bool = False, parallel: bool = False, from_source: bool = False, source_url: str | None = None, version: str | None = None, - ): + ) -> int: + """Install software packages via Cortex. + + Args: + software: Package name(s) or install request string. + execute: Execute generated commands. + dry_run: Show commands without executing. + parallel: Run commands in parallel when supported. + from_source: Build/install from source when binaries are unavailable. + source_url: Optional source URL override. + version: Optional version to build/install. + """As per coding guidelines, public APIs should include docstrings and type hints.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cortex/cli.pycortex/source_builder.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide for Python code
Include type hints in Python code
Add docstrings for all public APIs in Python code
Use dry-run mode by default for all installation operations
Do not use silent sudo - require explicit user confirmation for privilege escalation
Implement Firejail sandboxing for execution of untrusted code
Log all operations to ~/.cortex/history.db for audit purposes
Files:
cortex/cli.pycortex/source_builder.py
🧠 Learnings (3)
📚 Learning: 2026-01-12T20:51:13.828Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-12T20:51:13.828Z
Learning: Applies to **/*.py : Log all operations to ~/.cortex/history.db for audit purposes
Applied to files:
cortex/cli.py
📚 Learning: 2026-01-12T20:51:13.828Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-12T20:51:13.828Z
Learning: Applies to **/*.py : Use dry-run mode by default for all installation operations
Applied to files:
cortex/cli.py
📚 Learning: 2026-01-12T20:51:13.828Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-12T20:51:13.828Z
Learning: Applies to **/*.py : Implement Firejail sandboxing for execution of untrusted code
Applied to files:
cortex/cli.py
🧬 Code graph analysis (2)
cortex/cli.py (4)
cortex/coordinator.py (3)
execute(230-279)InstallationCoordinator(51-324)InstallationResult(43-48)cortex/sandbox/sandbox_executor.py (3)
execute(501-633)SandboxExecutor(99-662)success(74-76)cortex/source_builder.py (1)
build_from_source(566-774)cortex/installation_history.py (4)
record_installation(259-317)InstallationType(27-35)update_installation(319-368)InstallationStatus(38-44)
cortex/source_builder.py (3)
cortex/branding.py (1)
cx_print(62-82)cortex/dependency_resolver.py (1)
is_package_installed(101-104)cortex/utils/commands.py (1)
run_command(223-305)
🔇 Additional comments (7)
cortex/cli.py (6)
13-14: Logger initialization looks good.
245-248: Comment clarification is fine.
1528-1530: History output formatting looks good.
2119-2120: Message clarity improvement looks good.
3296-3310: CLI flag additions look good.
3881-3884: Argument wiring is consistent.cortex/source_builder.py (1)
52-110: Core dataclasses + cache init look solid.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| # Handle --from-source flag | ||
| if from_source: | ||
| return self._install_from_source(software, execute, dry_run, source_url, version) |
There was a problem hiding this comment.
--from-source runs real builds without --execute.
With from_source=True and execute=False, the flow still downloads, installs build deps, and compiles. This violates the “dry‑run by default” requirement for installation operations.
✅ Suggested guard
- if from_source:
- return self._install_from_source(software, execute, dry_run, source_url, version)
+ if from_source:
+ if not execute and not dry_run:
+ dry_run = True
+ return self._install_from_source(software, execute, dry_run, source_url, version)Based on learnings, dry‑run should be the default for install operations.
🤖 Prompt for AI Agents
In `@cortex/cli.py` around lines 860 - 862, The current branch returns into
_install_from_source(software, execute, dry_run, source_url, version) when
from_source is True but _install_from_source still performs downloads,
dependency installation and compilation even if execute is False; update
_install_from_source (referenced here) to honor the execute/dry_run flags: when
execute is False (or dry_run True) it must not perform any real side-effecting
steps (download, install build deps, run compile commands) and instead only log
or simulate the steps and return a successful dry-run result; ensure all
internal calls that spawn subprocesses or write files check the execute flag
first and that the top-level call from the from_source branch passes the correct
dry_run/execute flags through to enforce dry-run behavior.
| cx_print(f"📥 Downloading {package_name} source...", "info") | ||
| archive_path = temp_dir / "source.tar.gz" | ||
|
|
||
| # Modify GitHub URLs if needed (only after validation) | ||
| if url.startswith("https://github.com/"): | ||
| # GitHub release or archive | ||
| if not url.endswith((".tar.gz", ".zip")): | ||
| if version: | ||
| url = f"{url}/archive/refs/tags/v{version}.tar.gz" | ||
| else: | ||
| url = f"{url}/archive/refs/heads/main.tar.gz" | ||
| # Re-validate modified URL | ||
| self._validate_url(url) | ||
|
|
||
| urllib.request.urlretrieve(url, archive_path) | ||
|
|
||
| # Extract | ||
| cx_print("📦 Extracting source...", "info") | ||
| extract_dir = temp_dir / "extracted" | ||
| extract_dir.mkdir() | ||
|
|
||
| if archive_path.suffix == ".gz" or archive_path.suffixes[-2:] == [".tar", ".gz"]: |
There was a problem hiding this comment.
Archive is always saved as .tar.gz, breaking .zip downloads.
archive_path is hardcoded to source.tar.gz, so suffix detection always chooses the tar path even when the URL is a zip.
🐛 Suggested fix
- archive_path = temp_dir / "source.tar.gz"
+ parsed_path = Path(urllib.parse.urlparse(url).path)
+ suffix = "".join(parsed_path.suffixes) or ".tar.gz"
+ archive_path = temp_dir / f"source{suffix}"🤖 Prompt for AI Agents
In `@cortex/source_builder.py` around lines 300 - 321, The code always saves
downloads to a hardcoded archive_path = temp_dir / "source.tar.gz", which breaks
handling of .zip archives; change the download/save logic in the block around
archive_path, urllib.request.urlretrieve(...) and subsequent extraction code so
the downloaded filename/extension matches the URL (e.g., derive archive_path
from URL path or use tempfile with proper suffix), re-run self._validate_url as
before, and then branch extraction based on the actual saved file suffix
(reference symbols: archive_path, urllib.request.urlretrieve, extract_dir, and
the extraction conditional that checks archive_path.suffix / suffixes). Ensure
zip downloads are saved with .zip and extracted with the zip branch while tar.gz
keeps the tar extraction flow.
| # Detect and install build dependencies | ||
| cx_print("🔍 Checking build dependencies...", "info") | ||
| missing_deps = self.detect_build_dependencies(package_name, build_system) | ||
|
|
||
| if missing_deps: | ||
| cx_print(f" Installing: {', '.join(missing_deps)}", "info") | ||
| install_cmd = f"sudo apt-get install -y {' '.join(missing_deps)}" | ||
| result = run_command(install_cmd, timeout=600) | ||
| if not result.success: |
There was a problem hiding this comment.
Avoid silent sudo for build dependencies.
Auto‑installing deps via sudo apt-get install -y runs privileged operations without explicit confirmation.
As per coding guidelines, require explicit user confirmation for privilege escalation.
🤖 Prompt for AI Agents
In `@cortex/source_builder.py` around lines 657 - 665, The code auto-runs
privileged installs using install_cmd and run_command when missing_deps is
non-empty (detected by detect_build_dependencies) without prompting; change this
flow to request explicit user confirmation via cx_print and an interactive
prompt (or a passed-in flag) before constructing/running install_cmd, aborting
or returning an error if the user declines, and ensure the prompt mentions the
exact packages (missing_deps) and the use of sudo so callers of
detect_build_dependencies/install_cmd/run_command can opt out or provide
non‑privileged alternatives.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cortex/cli.py`:
- Around line 2936-2955: The current failure path in the build handling (the
block using result, history.record_installation, InstallationType.INSTALL) skips
writing to history when result.install_commands is empty; change the conditional
so we still record failed builds (using result.install_commands or an empty
list) whenever this is a real run (i.e., result.build_dir != "<dry-run-no-dir>")
— keep the try/except and history.update_installation call (use
error_message=result.error_message or "Build failed") and pass [] if
result.install_commands is falsy so the failure is always audited.
♻️ Duplicate comments (10)
cortex/cli.py (2)
864-868: Default--from-sourcepath should be dry-run unless--executeis set.Line 866 routes directly into
_install_from_sourceeven whenexecute=Falseanddry_run=False, which triggers a real build by default.Based on learnings, dry-run should be the default for install operations.🛠️ Suggested guard
- if from_source: - return self._install_from_source(software, execute, dry_run, source_url, version) + if from_source: + if not execute and not dry_run: + dry_run = True + return self._install_from_source(software, execute, dry_run, source_url, version)
3013-3026: Firejail is detected but not enforced for install commands.This still executes
result.install_commandsdirectly throughInstallationCoordinator, so builds/installations aren’t sandboxed despite the availability check.Please wire the SandboxExecutor into command execution (e.g., wrap commands or pass an executor into the coordinator if supported).
#!/bin/bash # Inspect sandbox executor API and coordinator execution hooks rg -n "class SandboxExecutor|def .*execute|def .*wrap|firejail" cortex/sandbox/sandbox_executor.py rg -n "class InstallationCoordinator|def execute" cortex/coordinator.py rg -n "sandbox" cortex/coordinator.pyBased on learnings, Firejail sandboxing is required for untrusted execution.
cortex/source_builder.py (8)
126-155: Cache metadata points to directories that may be deleted.
metadata["build_dir"]is persisted, butbuild_from_sourcelater deletes temp dirs; cached builds can therefore point to missing artifacts and be unusable.Consider either copying build artifacts into the cache directory or deferring cleanup when caching is enabled so
build_dirremains valid.
298-347: Archive download always saved as.tar.gz, breaking.zipsources.Line 301 hardcodes
source.tar.gz, so.zipdownloads are misclassified and the zip extraction path is never taken.🐛 Suggested fix
- archive_path = temp_dir / "source.tar.gz" + parsed_path = Path(urllib.parse.urlparse(url).path) + suffix = "".join(parsed_path.suffixes) or ".tar.gz" + archive_path = temp_dir / f"source{suffix}"
449-469: Autotools fallback can still call a missingconfigure.If only
configure.acexists, the code still setsconfigure_scripttoconfigurewithout generating it, which will fail at execution time.🛠️ Suggested fix
configure_script = source_dir / "configure" if not configure_script.exists(): - # Fallback: try configure.ac (needs autogen) - configure_script = source_dir / "configure.ac" - if configure_script.exists(): - # Would need autogen.sh first, but for now use configure - configure_script = source_dir / "configure" + configure_ac = source_dir / "configure.ac" + if configure_ac.exists(): + raise FileNotFoundError( + f"No 'configure' script found in {source_dir}. " + "Run autogen.sh/autoconf to generate it." + ) + raise FileNotFoundError(f"No 'configure' script found in {source_dir}.")
553-563: Install commands are cwd-sensitive and likely to fail.
sudo make installandsudo python3 setup.py installrequire running from the source/build dir, but_install_from_sourceexecutes them from the CLI cwd.🛠️ Suggested fix
- if config.build_system == "autotools" or config.build_system == "make": - commands.append("sudo make install") + if config.build_system == "autotools" or config.build_system == "make": + commands.append(f"sudo make -C {shlex.quote(str(source_dir))} install") @@ - elif config.build_system == "python": - commands.append("sudo python3 setup.py install") + elif config.build_system == "python": + commands.append(f"sudo python3 -m pip install {shlex.quote(str(source_dir))}")
657-665: Silent sudo for build dependencies.Line 663 runs
sudo apt-get install -y ...without explicit confirmation, which violates the privilege-escalation requirement.As per coding guidelines, require explicit user confirmation for privilege escalation.🛠️ Suggested fix
if missing_deps: - cx_print(f" Installing: {', '.join(missing_deps)}", "info") - install_cmd = f"sudo apt-get install -y {' '.join(missing_deps)}" - result = run_command(install_cmd, timeout=600) + cx_print(f" Installing: {', '.join(missing_deps)}", "info") + confirm = input( + f"Install build dependencies with sudo? ({', '.join(missing_deps)}) [y/N]: " + ).strip().lower() + if confirm not in ("y", "yes"): + return BuildResult( + success=False, + package_name=package_name, + version=version, + build_dir=str(source_dir), + install_commands=[], + error_message="User declined dependency installation.", + ) + install_cmd = f"sudo apt-get install -y {' '.join(missing_deps)}" + result = run_command(install_cmd, timeout=600)
677-710: Build/configure commands run unsandboxed.
run_command(...)executes untrusted build steps directly without Firejail.Please route build/configure commands through the SandboxExecutor (or a sandbox-aware run_command wrapper) so untrusted source builds are isolated.
#!/bin/bash # Inspect sandbox executor capabilities and usage points rg -n "class SandboxExecutor|def .*execute|firejail" cortex/sandbox/sandbox_executor.py rg -n "run_command" cortex/source_builder.pyBased on learnings, Firejail sandboxing is required for untrusted execution.
728-739: Cleanup happens before install commands run.The temp source/build directory is deleted immediately after build, but install commands (returned to CLI) still need those artifacts.
🛠️ Suggested fix
- # Clean up temporary directories after successful build - # Find and remove the parent temp directory if it's in /tmp - try: - source_path = source_dir.resolve() - if "/tmp" in str(source_path) or "/var/tmp" in str(source_path): - # Find the top-level temp directory created by cortex - temp_parent = source_path - while temp_parent.parent != temp_parent: # Stop at root - if "cortex-build-" in temp_parent.name: - # This is the main temp directory created by mkdtemp - shutil.rmtree(temp_parent, ignore_errors=True) - break - temp_parent = temp_parent.parent - except Exception as e: - logger.warning(f"Failed to clean up temporary directory: {e}") + # Defer cleanup until after installation succeeds (caller-managed).
752-756:source_dirmay be undefined on early failures.If
fetch_source(...)raises,source_dirisn’t assigned and the exception handler can throwUnboundLocalError.🛠️ Suggested fix
- try: + source_dir: Path | None = None + try: # Handle dry-run mode: skip actual build and return planned result
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cortex/cli.pycortex/source_builder.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide for Python code
Include type hints in Python code
Add docstrings for all public APIs in Python code
Use dry-run mode by default for all installation operations
Do not use silent sudo - require explicit user confirmation for privilege escalation
Implement Firejail sandboxing for execution of untrusted code
Log all operations to ~/.cortex/history.db for audit purposes
Files:
cortex/cli.pycortex/source_builder.py
🧠 Learnings (4)
📚 Learning: 2026-01-12T20:51:13.828Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-12T20:51:13.828Z
Learning: Applies to **/*.py : Log all operations to ~/.cortex/history.db for audit purposes
Applied to files:
cortex/cli.py
📚 Learning: 2026-01-12T20:51:13.828Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-12T20:51:13.828Z
Learning: Applies to **/*.py : Use dry-run mode by default for all installation operations
Applied to files:
cortex/cli.py
📚 Learning: 2026-01-12T20:51:13.828Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-12T20:51:13.828Z
Learning: Applies to **/*.py : Implement Firejail sandboxing for execution of untrusted code
Applied to files:
cortex/cli.py
📚 Learning: 2026-01-12T20:51:13.828Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-12T20:51:13.828Z
Learning: Applies to **/*.py : Do not use silent sudo - require explicit user confirmation for privilege escalation
Applied to files:
cortex/source_builder.py
🧬 Code graph analysis (2)
cortex/cli.py (4)
cortex/coordinator.py (5)
InstallationCoordinator(51-324)InstallationResult(43-48)InstallationStep(26-39)StepStatus(17-22)execute(230-279)cortex/sandbox/sandbox_executor.py (3)
execute(501-633)SandboxExecutor(99-662)success(74-76)cortex/source_builder.py (2)
SourceBuilder(80-774)build_from_source(566-774)cortex/installation_history.py (3)
InstallationHistory(75-633)record_installation(259-317)update_installation(319-368)
cortex/source_builder.py (3)
cortex/branding.py (1)
cx_print(62-82)cortex/dependency_resolver.py (1)
is_package_installed(101-104)cortex/utils/commands.py (1)
run_command(223-305)
🔇 Additional comments (15)
cortex/cli.py (7)
13-23: Coordinator import update looks good.InstallationResult is now co-located with related coordinator types, which clarifies usage.
248-253: No action needed here.
822-831: Install signature extension looks good.The new
from_source,source_url, andversionparameters are wired cleanly.
1533-1535: History table formatting change looks fine.
2124-2125: Error message tweak looks good.
3260-3274: CLI flag wiring looks good.
3845-3848: Main install call correctly forwards new flags.cortex/source_builder.py (8)
1-37: Module header/imports are clear and PEP 8 compliant.
52-78: Dataclass definitions look good.
92-110: Cache dir fallback handling is solid.
156-189: Dependency detection logic is clear.
191-281: Fetch/URL validation flow looks good.
384-402: No action needed here.
404-431: Build-system detection looks good.
506-535: Build command generation looks fine.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| if not result.success: | ||
| self._print_error(f"Build failed: {result.error_message}") | ||
| # Record failed installation (only if we have real install commands) | ||
| if result.install_commands and result.build_dir != "<dry-run-no-dir>": | ||
| try: | ||
| install_id = history.record_installation( | ||
| InstallationType.INSTALL, | ||
| [package_name], | ||
| result.install_commands, | ||
| start_time, | ||
| ) | ||
| history.update_installation( | ||
| install_id, | ||
| InstallationStatus.FAILED, | ||
| error_message=result.error_message or "Build failed", | ||
| ) | ||
| except Exception as e: | ||
| logger.warning(f"Failed to record installation failure: {e}") | ||
| cx_print(f"⚠️ Warning: Could not record installation failure: {e}", "warning") | ||
| return 1 |
There was a problem hiding this comment.
Record failed builds in history even when install commands are empty.
If the build fails before install_commands are known, the current conditional skips history writes entirely, so failures aren’t audited.
🛠️ Suggested fix
- if not result.success:
- self._print_error(f"Build failed: {result.error_message}")
- # Record failed installation (only if we have real install commands)
- if result.install_commands and result.build_dir != "<dry-run-no-dir>":
- try:
- install_id = history.record_installation(
- InstallationType.INSTALL,
- [package_name],
- result.install_commands,
- start_time,
- )
- history.update_installation(
- install_id,
- InstallationStatus.FAILED,
- error_message=result.error_message or "Build failed",
- )
- except Exception as e:
- logger.warning(f"Failed to record installation failure: {e}")
- cx_print(f"⚠️ Warning: Could not record installation failure: {e}", "warning")
- return 1
+ if not result.success:
+ self._print_error(f"Build failed: {result.error_message}")
+ try:
+ install_id = history.record_installation(
+ InstallationType.INSTALL,
+ [package_name],
+ result.install_commands or [],
+ start_time,
+ )
+ history.update_installation(
+ install_id,
+ InstallationStatus.FAILED,
+ error_message=result.error_message or "Build failed",
+ )
+ except Exception as e:
+ logger.warning(f"Failed to record installation failure: {e}")
+ cx_print(f"⚠️ Warning: Could not record installation failure: {e}", "warning")
+ return 1🤖 Prompt for AI Agents
In `@cortex/cli.py` around lines 2936 - 2955, The current failure path in the
build handling (the block using result, history.record_installation,
InstallationType.INSTALL) skips writing to history when result.install_commands
is empty; change the conditional so we still record failed builds (using
result.install_commands or an empty list) whenever this is a real run (i.e.,
result.build_dir != "<dry-run-no-dir>") — keep the try/except and
history.update_installation call (use error_message=result.error_message or
"Build failed") and pass [] if result.install_commands is falsy so the failure
is always audited.
|
@aybanda Thanks for the contribution. The issue has been migrated to Please open a new PR against |
3 participants
Summary
Implements the ability to build and install packages from source code when binaries are unavailable. This feature includes automatic build dependency detection, build system detection, build caching, and comprehensive error handling.
Related Issue
Closes cortexlinux/cortex-distro#37
Type of Change
AI Disclosure
AI Tools Used:
Changes Made
Core Implementation
`cortex/source_builder.py - Core source building module
cortex/cli.py- CLI integration--from-sourceflag to install command--source-urloption for custom source locations--versionoption for version pinning_install_from_source()methodtests/test_source_builder.py- Comprehensive test suitedocs/SOURCE_BUILD.md- Complete documentationFeatures
Testing
Test Coverage
tests/test_source_builder.pyManual Testing
Code Quality
Documentation
docs/SOURCE_BUILD.mdwith:Example Usage
Security Considerations
run_commandutility with validationSummary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.