feat: add package build from source (#101) #455
Conversation
|
Warning Rate limit exceeded@aybanda has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 21 minutes and 15 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a build-from-source feature: new SourceBuilder subsystem (fetch, detect, configure, build, install, cache), CLI flags and wiring for Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant CLI as CortexCLI
participant Builder as SourceBuilder
participant Resolver as DependencyResolver
participant Coordinator as InstallationCoordinator
User->>CLI: install(pkg, from_source=True, source_url?, version?)
CLI->>Builder: build_from_source(pkg, version, source_url)
alt Cache Hit
Builder-->>CLI: BuildResult(cached=True, install_commands)
else Cache Miss
Builder->>Builder: fetch_source(...)
Builder->>Builder: detect_build_system(...)
Builder->>Resolver: detect_build_dependencies(pkg, build_system)
alt Missing deps
Resolver->>Resolver: attempt install/resolve dependencies
end
Builder->>Builder: configure_build(...)
Builder->>Builder: build(...)
Builder->>Builder: install_build(...)
Builder->>Builder: save_to_cache(...)
Builder-->>CLI: BuildResult(success, install_commands)
end
alt Execute Mode
CLI->>Coordinator: run_install_commands(install_commands, progress_cb)
Coordinator-->>CLI: success/failure
CLI->>CLI: update history, report result
else Dry-Run Mode
CLI-->>User: print planned install commands (no execute)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
CLA Verification FailedThe following contributors have not signed the Contributor License Agreement:
How to Sign
This check runs automatically. Maintainers can update |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (9)
cortex/cli.py (3)
1533-1551: Good implementation with proper type hints and docstring.The version parsing from
package_name@versionsyntax is a nice UX touch. However, there's a potential issue: if the user provides bothpackage@versionsyntax AND the--versionflag, the@versionwill silently override the flag due to theand not versioncondition. Consider warning the user about this.🔎 Optional: Warn on conflicting version specifications
# 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 else None + elif "@" in package_name and version: + cx_print( + f"Warning: Both @version syntax and --version flag provided. Using --version={version}", + "warning" + ) + package_name = package_name.split("@")[0]
1583-1584: Redundant import statement.
InstallationCoordinator,InstallationStep, andStepStatusare already imported at the top of the file (line 11). This local import is unnecessary.🔎 Remove redundant import
# Execute install commands - from cortex.coordinator import InstallationCoordinator, InstallationStep, StepStatus - def progress_callback(current: int, total: int, step: InstallationStep) -> None:
1594-1600: Consider more descriptive step descriptions.Using the same description for all install commands (
f"Install {package_name}") makes it harder for users to distinguish between steps in multi-command installs (e.g.,cd build && sudo make installvssudo make install).🔎 Use more descriptive step descriptions
coordinator = InstallationCoordinator( commands=result.install_commands, - descriptions=[f"Install {package_name}" for _ in result.install_commands], + descriptions=[f"Install {package_name}: {cmd[:50]}..." if len(cmd) > 50 else f"Install {package_name}: {cmd}" for cmd in result.install_commands], timeout=600, stop_on_error=True, progress_callback=progress_callback, )docs/SOURCE_BUILD.md (1)
78-82: Python build documentation could be more current.The documentation mentions
python3 setup.py build/install, but modern Python packaging preferspip install .withpyproject.toml. Consider noting that the current implementation uses the legacysetup.pyapproach.tests/test_source_builder.py (1)
175-182: Test assertion could be more specific.The assertion
"python3 setup.py install" in commands[0]passes for"sudo python3 setup.py install"but the actual implementation returns exactly that. Consider testing for the exact command or using a more precise assertion.🔎 More precise assertion
def test_install_build_python(self): """Test install commands for python.""" with tempfile.TemporaryDirectory() as tmpdir: source_dir = Path(tmpdir) config = BuildConfig(package_name="test", build_system="python") commands = self.builder.install_build(source_dir, config) assert len(commands) > 0 - assert "python3 setup.py install" in commands[0] + assert commands[0] == "sudo python3 setup.py install"cortex/source_builder.py (4)
32-34: Module-level side effect creates directory on import.Creating
CACHE_DIRat module import time causes a side effect. If the import fails after creating the directory, it leaves artifacts. Consider lazy initialization in__init__.🔎 Move directory creation to __init__
# Build cache directory CACHE_DIR = Path.home() / ".cortex" / "build_cache" -CACHE_DIR.mkdir(parents=True, exist_ok=True)Then in
__init__:def __init__(self): self.dependency_resolver = DependencyResolver() self.cache_dir = CACHE_DIR self.cache_dir.mkdir(parents=True, exist_ok=True) # Already present
14-16: Unused imports detected.
re,shutil, andsubprocessare imported but not used in the code. Thevalidate_commandimport fromcortex.utils.commandsalso appears unused.🔎 Remove unused imports
import hashlib import json import logging import os -import re -import shutil -import subprocess import tarfile import tempfile import urllib.request import zipfile ... from cortex.branding import cx_print from cortex.dependency_resolver import DependencyResolver -from cortex.utils.commands import CommandResult, run_command, validate_command +from cortex.utils.commands import run_commandAlso applies to: 28-28
326-330: Import inside function is repeated.
multiprocessingis imported twice inside thebuild()method. Move it to the top of the file with other imports for consistency and slight performance improvement.🔎 Move import to top level
+import multiprocessing import hashlib import json ... 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}"Also applies to: 338-342
443-461: Dependency installation uses hardcodedapt-get.The dependency installation assumes Debian/Ubuntu with
apt-get. This limits portability to other Linux distributions. Consider abstracting the package manager or documenting this limitation.This is acceptable for an initial implementation, but consider adding distribution detection for future enhancements. The limitation should be documented in
docs/SOURCE_BUILD.md.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cortex/cli.pycortex/source_builder.pydocs/SOURCE_BUILD.mdtests/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:
cortex/source_builder.pycortex/cli.pytests/test_source_builder.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_source_builder.py
🧬 Code graph analysis (1)
cortex/cli.py (5)
cortex/coordinator.py (4)
execute(230-279)InstallationCoordinator(51-324)InstallationStep(26-39)StepStatus(17-22)cortex/source_builder.py (2)
SourceBuilder(78-517)build_from_source(374-517)cortex/branding.py (1)
cx_print(49-69)cortex/first_run_wizard.py (1)
_print_error(746-748)cortex/semantic_cache.py (1)
total(32-34)
🔇 Additional comments (12)
cortex/cli.py (3)
546-549: LGTM! Well-structured signature extension.The new parameters
from_source,source_url, andversionare properly typed and have sensible defaults. The method signature follows existing patterns in the codebase.
583-587: Early return pattern is appropriate here.The
--from-sourceflow correctly bypasses the LLM-based command generation path. This is the right design choice since source builds have a completely different workflow.
1989-1991: Safe argument retrieval using getattr.Using
getattrwith defaults ensures backward compatibility if these arguments are not present.docs/SOURCE_BUILD.md (1)
1-242: Comprehensive and well-organized documentation.The documentation covers all essential aspects: usage patterns, supported build systems, dependencies, caching, troubleshooting, and limitations. The examples are practical and align with the CLI implementation. Good job!
tests/test_source_builder.py (4)
1-21: Good test file structure with appropriate imports.The imports are well-organized and the test file follows pytest conventions. Type hints via
from __future__ import annotationsis good practice.
35-46: Solid cache key generation tests.Tests verify determinism (same inputs → same key), uniqueness (different inputs → different keys), and key length. This covers the essential properties of a cache key.
76-82: Good negative test case for dependency detection.Testing that already-installed dependencies return an empty list ensures the dependency resolver doesn't suggest redundant installations.
286-308: Good coverage of BUILD_DEPENDENCIES constant.Testing the structure and contents of the dependency constants ensures they don't accidentally get broken by future changes.
cortex/source_builder.py (4)
50-76: Well-designed dataclasses with appropriate defaults.
BuildConfigandBuildResultare clean dataclasses with sensible defaults and clear field names. The type hints are complete.
78-93: Good class docstring with attributes documentation.The
SourceBuilderclass has a comprehensive docstring explaining its purpose and attributes. This follows Python docstring conventions.
374-422: Cache hit path is well-implemented.The cache check and early return with cached metadata is efficient. The
cached=Trueflag in the result helps callers know they're using cached data.
508-517: Exception handling is appropriate.The broad
except Exceptioncatch here is acceptable since it's the top-level orchestration method and returns a proper error result rather than crashing.
| install_parser.add_argument( | ||
| "--from-source", | ||
| action="store_true", | ||
| help="Build and install from source code when binaries unavailable", | ||
| ) | ||
| install_parser.add_argument( | ||
| "--source-url", | ||
| type=str, | ||
| help="URL to source code (for --from-source)", | ||
| ) | ||
| install_parser.add_argument( | ||
| "--version", | ||
| type=str, | ||
| help="Version to build (for --from-source)", | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify there's no conflict between global --version and install --version
grep -n "add_argument.*--version" cortex/cli.pyRepository: cortexlinux/cortex
Length of output: 156
🏁 Script executed:
#!/bin/bash
# Check parser structure and subparsers
sed -n '1700,1710p' cortex/cli.py
echo "---"
sed -n '1720,1750p' cortex/cli.pyRepository: cortexlinux/cortex
Length of output: 1745
🏁 Script executed:
#!/bin/bash
# Check how the CLI is actually used/tested and if --version conflict is a real issue
# Search for any tests or usage of 'install --version'
rg -n "install.*--version|--version.*install" cortex/ || echo "No install --version usage found"
echo "---"
# Check if there's any custom handling of version
sed -n '1698,1712p' cortex/cli.pyRepository: cortexlinux/cortex
Length of output: 757
Fix the --version argument conflict in the install subparser.
The global --version flag (line 1705) uses action="version", which intercepts all --version invocations at the main parser level before subparser arguments are processed. Running cortex install --version will print the cortex version and exit instead of passing the version argument to the install subparser.
Rename the install subparser's --version to something like --pkg-version or --target-version to avoid this conflict, or remove the global action="version" and handle versioning manually to allow subparsers to define their own --version arguments.
🤖 Prompt for AI Agents
In cortex/cli.py around lines 1733 to 1747 the install subparser defines a
--version argument that conflicts with the global parser's --version
(action="version"); change the install subparser argument name to avoid the
conflict (for example --pkg-version or --target-version) and update any
downstream references to use the new name so that cortex install
--pkg-version=<ver> supplies the intended value while the global --version
continues to print the CLI version.
| else: | ||
| url = f"{url}/archive/refs/heads/main.tar.gz" | ||
|
|
||
| urllib.request.urlretrieve(url, archive_path) |
There was a problem hiding this comment.
Network request lacks timeout and SSL verification control.
urllib.request.urlretrieve doesn't support timeout configuration and uses default SSL settings. Large files or unresponsive servers could hang indefinitely.
🔎 Use urllib.request.urlopen with timeout
- urllib.request.urlretrieve(url, archive_path)
+ # Use urlopen with timeout for better control
+ import ssl
+ context = ssl.create_default_context()
+ request = urllib.request.Request(url, headers={"User-Agent": "Cortex-SourceBuilder/1.0"})
+ with urllib.request.urlopen(request, timeout=300, context=context) as response:
+ with open(archive_path, "wb") as f:
+ # Read in chunks to handle large files
+ while chunk := response.read(8192):
+ f.write(chunk)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In cortex/source_builder.py around line 199, replace the direct use of
urllib.request.urlretrieve (which has no timeout or SSL control) with
urllib.request.urlopen using an explicit timeout and an SSLContext (or allow
passing a verify flag) and stream the response to archive_path in chunks; ensure
you create an ssl.SSLContext() (or use ssl.create_default_context()) to
enforce/disable verification according to configuration, wrap the urlopen call
in try/except to handle timeouts and network errors, and properly close the
response and file (or use a with-statement) so the download won't hang
indefinitely.
| if archive_path.suffix == ".gz" or archive_path.suffixes[-2:] == [".tar", ".gz"]: | ||
| with tarfile.open(archive_path, "r:gz") as tar: | ||
| tar.extractall(extract_dir) | ||
| elif archive_path.suffix == ".zip": | ||
| with zipfile.ZipFile(archive_path, "r") as zip_ref: | ||
| zip_ref.extractall(extract_dir) |
There was a problem hiding this comment.
Security: Archive extraction without path validation (Zip Slip vulnerability).
Both tarfile.extractall() and zipfile.extractall() are vulnerable to path traversal attacks where malicious archives can write files outside the extraction directory. For tarfile, Python 3.12+ has a filter parameter; for older versions, manual validation is needed.
🔎 Add path traversal protection
if archive_path.suffix == ".gz" or archive_path.suffixes[-2:] == [".tar", ".gz"]:
with tarfile.open(archive_path, "r:gz") as tar:
- tar.extractall(extract_dir)
+ # Python 3.12+ has filter parameter for security
+ # For compatibility, manually validate paths
+ for member in tar.getmembers():
+ member_path = Path(extract_dir) / member.name
+ if not member_path.resolve().is_relative_to(extract_dir.resolve()):
+ raise ValueError(f"Path traversal detected in archive: {member.name}")
+ tar.extractall(extract_dir)
elif archive_path.suffix == ".zip":
with zipfile.ZipFile(archive_path, "r") as zip_ref:
- zip_ref.extractall(extract_dir)
+ for member in zip_ref.namelist():
+ member_path = Path(extract_dir) / member
+ if not member_path.resolve().is_relative_to(extract_dir.resolve()):
+ raise ValueError(f"Path traversal detected in archive: {member}")
+ zip_ref.extractall(extract_dir)🤖 Prompt for AI Agents
In cortex/source_builder.py around lines 206 to 211, the code calls
tarfile.extractall() and zipfile.extractall() directly which is vulnerable to
Zip Slip (path traversal); change extraction to validate each member path before
writing: for tarfiles use tarfile.extractall(filter=...) on Python 3.12+ or
iterate tar.getmembers(), compute the resolved target path for each member by
joining it with extract_dir, ensure the resolved path is inside extract_dir (no
absolute paths or “..” escapes), and only then extract or raise on violations;
for zipfiles iterate zip.namelist()/infolist(), reject names that are absolute
or contain “..”, compute the resolved target path and ensure it starts with
extract_dir, create parent dirs as needed and write files safely; abort with an
error if any entry is unsafe.
| # Default configure options | ||
| configure_cmd += f" --prefix={config.install_prefix}" | ||
| configure_cmd += " --enable-optimizations" |
There was a problem hiding this comment.
--enable-optimizations is Python-specific, not generic autotools.
The --enable-optimizations flag is specific to Python's configure script and will cause errors for other autotools-based packages. This should only be added for known packages.
🔎 Remove or conditionally apply optimization flag
else:
# Default configure options
configure_cmd += f" --prefix={config.install_prefix}"
- configure_cmd += " --enable-optimizations"
+ # Note: --enable-optimizations is Python-specific
+ # Only add it for packages known to support it
+ if config.package_name.lower() == "python":
+ configure_cmd += " --enable-optimizations"
commands.append(configure_cmd)📝 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.
| # Default configure options | |
| configure_cmd += f" --prefix={config.install_prefix}" | |
| configure_cmd += " --enable-optimizations" | |
| # Default configure options | |
| configure_cmd += f" --prefix={config.install_prefix}" | |
| # Note: --enable-optimizations is Python-specific | |
| # Only add it for packages known to support it | |
| if config.package_name.lower() == "python": | |
| configure_cmd += " --enable-optimizations" |
🤖 Prompt for AI Agents
In cortex/source_builder.py around lines 287-289, the configure command
unconditionally appends the Python-specific flag "--enable-optimizations";
remove that unconditional append and only add this flag when building a known
Python package. Change the logic so the configure_cmd modification is
conditional (e.g., check package metadata/name or a build-language field
indicating Python) and append "--enable-optimizations" only when that check
passes; otherwise leave the generic autotools flags untouched. Ensure any new
conditional uses existing package/config properties (avoid hard-coding paths)
and include a brief comment explaining why the flag is Python-specific.
| ## See Also | ||
|
|
||
| - [Installation Guide](Getting-Started.md) | ||
| - [Dependency Resolution](README_DEPENDENCIES.md) | ||
| - [CLI Reference](COMMANDS.md) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if referenced documentation files exist
fd -t f "Getting-Started.md" docs/
fd -t f "README_DEPENDENCIES.md" docs/
fd -t f "COMMANDS.md" docs/Repository: cortexlinux/cortex
Length of output: 145
Fix broken documentation links with correct relative paths.
The "See Also" section references files that exist but at incorrect paths. Update the links to:
guides/Getting-Started.md(currentlyGetting-Started.md)modules/README_DEPENDENCIES.md(currentlyREADME_DEPENDENCIES.md)COMMANDS.md(correct)
🤖 Prompt for AI Agents
In docs/SOURCE_BUILD.md around lines 237 to 242, the "See Also" links point to
incorrect relative files; update the links so they reference
guides/Getting-Started.md instead of Getting-Started.md,
modules/README_DEPENDENCIES.md instead of README_DEPENDENCIES.md, and leave
COMMANDS.md unchanged; ensure the markdown link targets are updated accordingly
and verify paths resolve from docs/ directory.
| @patch("cortex.source_builder.run_command") | ||
| @patch("cortex.source_builder.urllib.request.urlretrieve") | ||
| @patch("cortex.source_builder.tarfile.open") | ||
| def test_fetch_from_url_tarball(self, mock_tarfile, mock_urlretrieve, mock_run_command): | ||
| """Test fetching source from URL (tarball).""" | ||
| # Mock tarfile extraction | ||
| mock_tar = MagicMock() | ||
| mock_tarfile.return_value.__enter__.return_value = mock_tar | ||
|
|
||
| with tempfile.TemporaryDirectory() as tmpdir: | ||
| # Create a mock extracted directory structure | ||
| extract_dir = Path(tmpdir) / "extracted" | ||
| extract_dir.mkdir() | ||
| source_subdir = extract_dir / "source-1.0" | ||
| source_subdir.mkdir() | ||
|
|
||
| # Mock the tarfile to return our structure | ||
| def mock_extractall(path): | ||
| (Path(path) / "source-1.0").mkdir(parents=True) | ||
|
|
||
| mock_tar.extractall = mock_extractall | ||
|
|
||
| result = self.builder._fetch_from_url( | ||
| "https://example.com/test.tar.gz", "test", "1.0" | ||
| ) | ||
| assert result is not None |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the source_builder.py file and examine the _fetch_from_url implementation
find . -name "source_builder.py" -type f | head -5Repository: cortexlinux/cortex
Length of output: 88
🏁 Script executed:
#!/bin/bash
# Search for _fetch_from_url method definition
rg -n "_fetch_from_url" --type py -A 30Repository: cortexlinux/cortex
Length of output: 8453
🏁 Script executed:
#!/bin/bash
# Also examine the test file to see the full context and implementation
fd "test_source_builder.py" -type fRepository: cortexlinux/cortex
Length of output: 232
🏁 Script executed:
#!/bin/bash
# Get the complete _fetch_from_url implementation
cat -n cortex/source_builder.py | sed -n '182,230p'Repository: cortexlinux/cortex
Length of output: 2671
🏁 Script executed:
#!/bin/bash
# Check what happens with urlretrieve - does it need to be mocked?
rg -n "urlretrieve" cortex/source_builder.py -B 2 -A 2Repository: cortexlinux/cortex
Length of output: 233
🏁 Script executed:
#!/bin/bash
# Check what run_command is and if it's needed in _fetch_from_url
rg -n "run_command" cortex/source_builder.py | head -20Repository: cortexlinux/cortex
Length of output: 389
🏁 Script executed:
#!/bin/bash
# Check if run_command is called in _fetch_from_url
sed -n '182,223p' cortex/source_builder.py | grep -i "run_command"Repository: cortexlinux/cortex
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Look at the full test class to see if there's a setUp or similar
sed -n '1,200p' tests/test_source_builder.py | grep -A 20 "class TestSourceBuilder"Repository: cortexlinux/cortex
Length of output: 925
Remove unused test setup and unnecessary mock patch; patch tempfile.mkdtemp to control temp directory behavior.
The test creates a tmpdir via tempfile.TemporaryDirectory() and sets up extract_dir within it, but these are unused—_fetch_from_url creates its own temp directory via tempfile.mkdtemp(). While mock_extractall creates "source-1.0" at the correct location (in the actual extract directory), the test setup is confusing.
Additionally, mock_run_command is patched but never called by _fetch_from_url.
To improve clarity: remove the unused tmpdir setup, drop the mock_run_command patch, and patch tempfile.mkdtemp to control and assert on the temp directory path used by the method.
🤖 Prompt for AI Agents
tests/test_source_builder.py around lines 184 to 209: the test unnecessarily
creates a TemporaryDirectory and patches run_command even though _fetch_from_url
uses tempfile.mkdtemp and never calls run_command; remove the TemporaryDirectory
setup and the mock_run_command patch, instead patch tempfile.mkdtemp to return a
controlled directory path, ensure your mocked tarfile.extractall creates the
"source-1.0" inside that returned temp dir, and adjust assertions to verify the
function used the patched tempdir (and that result is not None).
| def test_build_from_source_success(self): | ||
| """Test successful build_from_source.""" | ||
| with tempfile.TemporaryDirectory() as tmpdir: | ||
| source_dir = Path(tmpdir) | ||
| (source_dir / "configure").touch() | ||
|
|
||
| with patch.object( | ||
| self.builder, "fetch_source", return_value=source_dir | ||
| ), patch.object( | ||
| self.builder, "detect_build_dependencies", return_value=[] | ||
| ), patch( | ||
| "cortex.source_builder.run_command" | ||
| ) as mock_run: | ||
| # Mock successful commands | ||
| mock_run.return_value = Mock(success=True, stdout="", stderr="") | ||
|
|
||
| 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 |
There was a problem hiding this comment.
Test assertion is weak - doesn't verify success.
The test test_build_from_source_success only asserts result is not None (line 253), but doesn't verify result.success == True. This could mask failures where the build returns a failed result.
🔎 Strengthen the assertion
mock_run.return_value = Mock(success=True, stdout="", stderr="")
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 with all commands passing
+ assert result is not None
+ assert result.success is True
+ assert result.package_name == "test-package"📝 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 test_build_from_source_success(self): | |
| """Test successful build_from_source.""" | |
| with tempfile.TemporaryDirectory() as tmpdir: | |
| source_dir = Path(tmpdir) | |
| (source_dir / "configure").touch() | |
| with patch.object( | |
| self.builder, "fetch_source", return_value=source_dir | |
| ), patch.object( | |
| self.builder, "detect_build_dependencies", return_value=[] | |
| ), patch( | |
| "cortex.source_builder.run_command" | |
| ) as mock_run: | |
| # Mock successful commands | |
| mock_run.return_value = Mock(success=True, stdout="", stderr="") | |
| 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 | |
| def test_build_from_source_success(self): | |
| """Test successful build_from_source.""" | |
| with tempfile.TemporaryDirectory() as tmpdir: | |
| source_dir = Path(tmpdir) | |
| (source_dir / "configure").touch() | |
| with patch.object( | |
| self.builder, "fetch_source", return_value=source_dir | |
| ), patch.object( | |
| self.builder, "detect_build_dependencies", return_value=[] | |
| ), patch( | |
| "cortex.source_builder.run_command" | |
| ) as mock_run: | |
| # Mock successful commands | |
| mock_run.return_value = Mock(success=True, stdout="", stderr="") | |
| result = self.builder.build_from_source("test-package", use_cache=False) | |
| # Should succeed with all commands passing | |
| assert result is not None | |
| assert result.success is True | |
| assert result.package_name == "test-package" |
🤖 Prompt for AI Agents
In tests/test_source_builder.py around lines 235 to 253, the test only asserts
result is not None which is too weak; update the assertion to explicitly verify
the build succeeded by asserting result.success is True (and optionally check
result.stderr or stdout for expected messages). Modify the final assertion to
assert result.success is True (replace or add to the existing assert) so the
test fails if the build returned a failed result.
- Add SourceBuilder class for building packages from source - Support autotools, cmake, make, and python build systems - Add --from-source flag to install command - Implement build dependency detection and caching - Add comprehensive tests and documentation
aybanda
force-pushed
the
feature/source-build-from-source
branch
from
bd20fe8 to
4b3a5a3
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (7)
tests/test_source_builder.py (2)
184-209: Remove unused test setup and unnecessary mock patch.The test creates a
tmpdirviatempfile.TemporaryDirectory()and patchesrun_command, but neither is used by_fetch_from_url. The method creates its own temp directory viatempfile.mkdtemp()and doesn't callrun_command.🔎 Simplified test
- @patch("cortex.source_builder.run_command") @patch("cortex.source_builder.urllib.request.urlretrieve") @patch("cortex.source_builder.tarfile.open") - def test_fetch_from_url_tarball(self, mock_tarfile, mock_urlretrieve, mock_run_command): + @patch("cortex.source_builder.tempfile.mkdtemp") + def test_fetch_from_url_tarball(self, mock_mkdtemp, mock_tarfile, mock_urlretrieve): """Test fetching source from URL (tarball).""" + import tempfile + # Control the temp directory + controlled_tmpdir = tempfile.mkdtemp(prefix="test-fetch-") + mock_mkdtemp.return_value = controlled_tmpdir + # Mock tarfile extraction mock_tar = MagicMock() mock_tarfile.return_value.__enter__.return_value = mock_tar - with tempfile.TemporaryDirectory() as tmpdir: - # Create a mock extracted directory structure - extract_dir = Path(tmpdir) / "extracted" - extract_dir.mkdir() - source_subdir = extract_dir / "source-1.0" - source_subdir.mkdir() - - # Mock the tarfile to return our structure - def mock_extractall(path): - (Path(path) / "source-1.0").mkdir(parents=True) - - mock_tar.extractall = mock_extractall + def mock_extractall(path): + (Path(path) / "source-1.0").mkdir(parents=True) - result = self.builder._fetch_from_url( - "https://example.com/test.tar.gz", "test", "1.0" - ) - assert result is not None + mock_tar.extractall = mock_extractall + + result = self.builder._fetch_from_url( + "https://example.com/test.tar.gz", "test", "1.0" + ) + assert result is not None + assert "source-1.0" in str(result)
235-253: Test assertion is weak - doesn't verify success.The test
test_build_from_source_successonly assertsresult is not Nonebut doesn't verifyresult.success == True. This could mask failures where the build returns a failed result.🔎 Strengthen the assertion
mock_run.return_value = Mock(success=True, stdout="", stderr="") 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 with all commands passing + assert result is not None + assert result.success is True + assert result.package_name == "test-package"docs/SOURCE_BUILD.md (1)
237-242: Fix broken documentation links with correct relative paths.The "See Also" section references files that exist but at incorrect paths based on a previous review. The links should be updated to use correct relative paths from the
docs/directory.cortex/source_builder.py (3)
199-199: Network request lacks timeout and SSL verification control.
urllib.request.urlretrievedoesn't support timeout configuration. Large files or unresponsive servers could hang indefinitely.
206-211: Security: Archive extraction without path validation (Zip Slip vulnerability).Both
tarfile.extractall()andzipfile.extractall()are vulnerable to path traversal attacks where malicious archives can write files outside the extraction directory.
287-289:--enable-optimizationsis Python-specific, not generic autotools.The
--enable-optimizationsflag is specific to Python's configure script and will cause errors for other autotools-based packages. This should only be added for known packages.cortex/cli.py (1)
1743-1747: Fix the--versionargument conflict in the install subparser.The global
--versionflag (line 1705) usesaction="version", which intercepts all--versioninvocations at the main parser level before subparser arguments are processed. Runningcortex install pkg --version 1.0will print the cortex version and exit instead of passing the version argument to the install subparser.🔎 Rename to avoid conflict
install_parser.add_argument( - "--version", + "--pkg-version", type=str, help="Version to build (for --from-source)", )Then update
_install_from_sourcecall and themain()routing:- version=getattr(args, "version", None), + version=getattr(args, "pkg_version", None),
🧹 Nitpick comments (1)
cortex/cli.py (1)
1533-1611: LGTM! Well-implemented source build integration.The
_install_from_sourcemethod properly:
- Parses
package@versionsyntax- Delegates to
SourceBuilder.build_from_source()- Handles dry-run/execute modes appropriately
- Uses
InstallationCoordinatorfor command execution with progress feedbackOne minor note: Line 1584 re-imports
InstallationCoordinator,InstallationStep,StepStatuswhich are already imported at the module level (line 11). This is harmless but unnecessary.🔎 Remove redundant import
# Execute install commands - from cortex.coordinator import InstallationCoordinator, InstallationStep, StepStatus + # InstallationCoordinator, InstallationStep, StepStatus already imported at module level def progress_callback(current: int, total: int, step: InstallationStep) -> None:
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cortex/cli.pycortex/source_builder.pydocs/SOURCE_BUILD.mdtests/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/cli.pycortex/source_builder.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_source_builder.py
🧬 Code graph analysis (3)
tests/test_source_builder.py (1)
cortex/source_builder.py (11)
BuildConfig(51-62)BuildResult(66-75)SourceBuilder(78-517)_get_cache_key(95-98)detect_build_dependencies(124-157)detect_build_system(241-268)configure_build(270-307)build(309-348)install_build(350-372)_fetch_from_url(182-222)build_from_source(374-517)
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(78-517)build_from_source(374-517)
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)
🪛 GitHub Actions: CI
cortex/source_builder.py
[error] 202-202: Ruff: F541 f-string without any placeholders.
🪛 GitHub Check: lint
cortex/source_builder.py
[failure] 479-479: Ruff (F541)
cortex/source_builder.py:479:22: F541 f-string without any placeholders
[failure] 464-464: Ruff (F541)
cortex/source_builder.py:464:22: F541 f-string without any placeholders
[failure] 461-461: Ruff (F541)
cortex/source_builder.py:461:26: F541 f-string without any placeholders
[failure] 444-444: Ruff (F541)
cortex/source_builder.py:444:22: F541 f-string without any placeholders
[failure] 202-202: Ruff (F541)
cortex/source_builder.py:202:22: F541 f-string without any placeholders
🪛 GitHub Check: Lint
cortex/source_builder.py
[failure] 479-479: Ruff (F541)
cortex/source_builder.py:479:22: F541 f-string without any placeholders
[failure] 464-464: Ruff (F541)
cortex/source_builder.py:464:22: F541 f-string without any placeholders
[failure] 461-461: Ruff (F541)
cortex/source_builder.py:461:26: F541 f-string without any placeholders
[failure] 444-444: Ruff (F541)
cortex/source_builder.py:444:22: F541 f-string without any placeholders
[failure] 202-202: Ruff (F541)
cortex/source_builder.py:202:22: F541 f-string without any placeholders
⏰ 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 (15)
docs/SOURCE_BUILD.md (1)
1-14: LGTM! Comprehensive documentation for the new feature.The overview clearly explains the source build feature's capabilities and aligns well with the implementation in
cortex/source_builder.py.tests/test_source_builder.py (6)
1-21: LGTM! Good test structure with proper imports.The test file is well-organized with proper imports from the source module. The use of
pytestandunittest.mockis appropriate for this test suite.
23-46: LGTM! Setup and cache key tests are well-designed.The
setup_methodprovides a clean fixture, and the cache key tests properly verify determinism (same inputs → same key) and uniqueness (different inputs → different keys).
48-82: LGTM! Dependency detection tests provide good coverage.Tests properly verify that the correct dependencies are detected for each build system (autotools, cmake, python) and that already-satisfied dependencies are excluded from the result.
84-117: LGTM! Build system detection tests are thorough.Good coverage of all detection scenarios with appropriate use of temporary directories and marker files to simulate different project types.
119-182: LGTM! Command generation tests verify expected output.Tests properly verify that configure, build, and install commands are generated correctly for different build systems.
256-311: LGTM! Dataclass and constant tests are well-structured.The
TestBuildConfigtests verify default values and custom configuration, whileTestBuildDependenciesvalidates the expected structure of theBUILD_DEPENDENCIESconstant.cortex/source_builder.py (5)
1-48: LGTM! Module header, imports, and constants are well-organized.Good use of type hints and proper organization of imports. The
BUILD_DEPENDENCIESdictionary provides a clear structure for categorizing build dependencies.
50-76: LGTM! Dataclasses are well-designed with appropriate defaults.
BuildConfigandBuildResultprovide clear data structures for the build workflow with sensible defaults and optional fields.
78-122: LGTM! Cache implementation is well-structured.The cache key generation uses SHA256 for collision resistance, and the cache check/save logic with the "installed" marker file is a reasonable approach for tracking build state.
309-372: LGTM! Build and install command generation is correct.The methods properly handle different build systems and use parallel jobs by default. The sudo prefix for install commands is appropriate.
374-518: LGTM overall! Thebuild_from_sourcemethod provides a comprehensive workflow.The orchestration logic is well-structured with proper error handling, caching support, and detailed BuildResult reporting. The method handles all phases (fetch, dependencies, configure, build, install) with appropriate timeouts and error messages.
Ensure all the f-string lint issues identified above are fixed to unblock the CI pipeline.
cortex/cli.py (3)
546-549: LGTM! Clean extension of the install method signature.The new parameters
from_source,source_url, andversionare properly typed and have sensible defaults.
583-587: LGTM! Clean routing to source build path.The early return pattern for
from_sourceis clean and keeps the existing installation logic unaffected.
1989-1991: LGTM! Proper argument forwarding in main().The new arguments are correctly retrieved with
getattrand forwarded tocli.install().
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cortex/source_builder.py (1)
227-243: Consider externalizing source URL mappings.The hardcoded
common_urlsdictionary provides a good starting point for common packages, but could be moved to a configuration file or registry for easier maintenance and extensibility.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cortex/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.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)
CommandResult(118-125)
🪛 GitHub Actions: CI
cortex/source_builder.py
[error] 205-205: Command failed: ruff check . --output-format=github. Issue: F541 f-string without placeholders in cortex/source_builder.py:205.
🪛 GitHub Check: lint
cortex/source_builder.py
[failure] 482-482: Ruff (F541)
cortex/source_builder.py:482:22: F541 f-string without any placeholders
[failure] 467-467: Ruff (F541)
cortex/source_builder.py:467:22: F541 f-string without any placeholders
[failure] 464-464: Ruff (F541)
cortex/source_builder.py:464:26: F541 f-string without any placeholders
[failure] 447-447: Ruff (F541)
cortex/source_builder.py:447:22: F541 f-string without any placeholders
[failure] 205-205: Ruff (F541)
cortex/source_builder.py:205:22: F541 f-string without any placeholders
🪛 GitHub Check: Lint
cortex/source_builder.py
[failure] 482-482: Ruff (F541)
cortex/source_builder.py:482:22: F541 f-string without any placeholders
[failure] 467-467: Ruff (F541)
cortex/source_builder.py:467:22: F541 f-string without any placeholders
[failure] 464-464: Ruff (F541)
cortex/source_builder.py:464:26: F541 f-string without any placeholders
[failure] 447-447: Ruff (F541)
cortex/source_builder.py:447:22: F541 f-string without any placeholders
[failure] 205-205: Ruff (F541)
cortex/source_builder.py:205:22: F541 f-string without any placeholders
⏰ 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 (6)
cortex/source_builder.py (6)
53-79: LGTM! Well-structured data models.The
BuildConfigandBuildResultdataclasses are clean, properly typed, and well-documented. The default values are sensible and the fields clearly represent the build workflow state.
81-126: LGTM! Clean caching implementation.The caching mechanism is well-designed with deterministic cache keys, metadata storage, and an "installed" marker for tracking build artifacts. The implementation follows best practices.
127-161: LGTM! Solid dependency detection logic.The method correctly categorizes dependencies by build system type and checks installation status through the
DependencyResolver. The pattern-based detection for package-specific dependencies is a good approach.
244-272: LGTM! Comprehensive build system detection.The detection logic correctly prioritizes explicit build system markers (configure scripts, CMakeLists.txt, etc.) and provides a sensible default. The fallback to autotools is reasonable given its prevalence.
353-376: LGTM! Clean install command generation.The method correctly generates install commands for each build system type, appropriately using
sudofor system-level installation.
377-521: LGTM! Well-structured main build workflow.The
build_from_sourcemethod implements a comprehensive build pipeline with proper error handling, caching, and progress reporting. The method correctly:
- Checks cache before rebuilding
- Orchestrates fetch, configure, build, and install phases
- Handles dependencies automatically
- Returns structured
BuildResultwith detailed error information- Uses appropriate timeouts for long-running operations
The exception handling ensures failures are captured and reported gracefully.
| def build(self, source_dir: Path, config: BuildConfig) -> list[str]: | ||
| """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. | ||
| """ | ||
| commands = [] | ||
|
|
||
| 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}" | ||
| 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}") | ||
|
|
||
| elif config.build_system == "python": | ||
| commands.append("python3 setup.py build") | ||
|
|
||
| return commands | ||
|
|
There was a problem hiding this comment.
Move multiprocessing import to module level.
The import multiprocessing statement appears inside the method at lines 330 and 342, violating PEP 8 guidelines. Module-level imports improve code clarity and avoid repeated import overhead.
🔎 Proposed fix
At the top of the file, add the import with other standard library imports:
import subprocess
import tarfile
import tempfile
+import multiprocessing
import urllib.request
import zipfileThen update the method to remove the inline imports:
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}"
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}"As per coding guidelines: Follow PEP 8 style guide.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In cortex/source_builder.py around lines 312 to 352, there are inline imports of
multiprocessing at lines ~330 and ~342; move the "import multiprocessing"
statement to the module-level imports at the top of the file (grouped with other
standard library imports) and remove the two inline import statements inside the
build() method so the code simply references multiprocessing.cpu_count() where
needed, preserving the existing logic for computing jobs and constructing
make_cmd.
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)
1-1: Fix Black formatting issues blocking CI.The Black formatter check is failing in CI, indicating the file needs to be reformatted. This is blocking the pipeline and must be fixed before merge.
Run Black to fix formatting:
#!/bin/bash # Fix Black formatting issues black cortex/cli.py
♻️ Duplicate comments (10)
cortex/cli.py (1)
1747-1750: Fix the--versionargument conflict in the install subparser.The global
--versionflag (line 1708) usesaction="version", which intercepts all--versioninvocations at the main parser level before subparser arguments are processed. Runningcortex install --version 3.12will print the cortex version and exit instead of passing the version argument to the install subparser.Rename the install subparser's
--versionto--pkg-versionor--package-versionto avoid this conflict.🔎 Rename to avoid conflict
install_parser.add_argument( - "--version", + "--package-version", type=str, - help="Version to build (for --from-source)", + help="Package version to build (for --from-source)", )Then update the main() function to use the new name:
elif args.command == "install": return cli.install( args.software, execute=args.execute, dry_run=args.dry_run, parallel=args.parallel, from_source=getattr(args, "from_source", False), source_url=getattr(args, "source_url", None), - version=getattr(args, "version", None), + version=getattr(args, "package_version", None), )cortex/source_builder.py (9)
333-333: Movemultiprocessingimport to module level.The
import multiprocessingstatement appears inside the method, violating PEP 8 guidelines. Module-level imports improve code clarity and avoid repeated import overhead.🔎 Move import to module level
At the top of the file (around line 23), add:
import subprocess import tarfile import tempfile +import multiprocessing import urllib.request import zipfileThen remove the inline imports at lines 333 and 345:
else: # Use parallel builds by default - import multiprocessing - jobs = multiprocessing.cpu_count() make_cmd += f" -j{jobs}"As per coding guidelines: Follow PEP 8 style guide.
202-202: Network request lacks timeout and SSL verification control.
urllib.request.urlretrievedoesn't support timeout configuration and uses default SSL settings. Large files or unresponsive servers could hang indefinitely.🔎 Use urllib.request.urlopen with timeout
- urllib.request.urlretrieve(url, archive_path) + # Use urlopen with timeout for better control + import ssl + context = ssl.create_default_context() + request = urllib.request.Request(url, headers={"User-Agent": "Cortex-SourceBuilder/1.0"}) + with urllib.request.urlopen(request, timeout=300, context=context) as response: + with open(archive_path, "wb") as f: + # Read in chunks to handle large files + while chunk := response.read(8192): + f.write(chunk)
209-214: Security: Archive extraction without path validation (Zip Slip vulnerability).Both
tarfile.extractall()andzipfile.extractall()are vulnerable to path traversal attacks where malicious archives can write files outside the extraction directory.🔎 Add path traversal protection
if archive_path.suffix == ".gz" or archive_path.suffixes[-2:] == [".tar", ".gz"]: with tarfile.open(archive_path, "r:gz") as tar: - tar.extractall(extract_dir) + # Validate paths to prevent Zip Slip + for member in tar.getmembers(): + member_path = Path(extract_dir) / member.name + try: + member_path.resolve().relative_to(extract_dir.resolve()) + except ValueError: + raise ValueError(f"Path traversal detected in archive: {member.name}") + tar.extractall(extract_dir) elif archive_path.suffix == ".zip": with zipfile.ZipFile(archive_path, "r") as zip_ref: - zip_ref.extractall(extract_dir) + # Validate paths to prevent Zip Slip + for member in zip_ref.namelist(): + member_path = Path(extract_dir) / member + try: + member_path.resolve().relative_to(extract_dir.resolve()) + except ValueError: + raise ValueError(f"Path traversal detected in archive: {member}") + zip_ref.extractall(extract_dir)
295-295:--enable-optimizationsis Python-specific, not generic autotools.The
--enable-optimizationsflag is specific to Python's configure script and will cause errors for other autotools-based packages. This should only be added for known Python packages.🔎 Make optimization flag conditional
else: # Default configure options configure_cmd += f" --prefix={config.install_prefix}" - configure_cmd += " --enable-optimizations" + # --enable-optimizations is Python-specific + if config.package_name.lower() in ("python", "python3"): + configure_cmd += " --enable-optimizations" commands.append(configure_cmd)
205-205: Fix f-string without placeholders (lint failure).This f-string has no placeholders and is causing the CI pipeline to fail. Use a regular string instead.
🔎 Remove unnecessary f-prefix
- cx_print(f"📦 Extracting source...", "info") + cx_print("📦 Extracting source...", "info")
450-450: Fix f-string without placeholders (lint failure).This f-string has no placeholders and is causing CI lint failures.
🔎 Remove unnecessary f-prefix
- cx_print(f"🔍 Checking build dependencies...", "info") + cx_print("🔍 Checking build dependencies...", "info")
467-467: Fix f-string without placeholders (lint failure).This f-string has no placeholders.
🔎 Remove unnecessary f-prefix
- cx_print(f" ✓ All build dependencies satisfied", "success") + cx_print(" ✓ All build dependencies satisfied", "success")
470-470: Fix f-string without placeholders (lint failure).This f-string has no placeholders.
🔎 Remove unnecessary f-prefix
- cx_print(f"⚙️ Configuring build...", "info") + cx_print("⚙️ Configuring build...", "info")
485-485: Fix f-string without placeholders (lint failure).This f-string has no placeholders.
🔎 Remove unnecessary f-prefix
- cx_print(f"🔨 Compiling (this may take a while)...", "info") + cx_print("🔨 Compiling (this may take a while)...", "info")
📜 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 (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)
🪛 GitHub Actions: CI
cortex/cli.py
[error] 1-1: Black formatting check failed. 1 file would be reformatted by 'black --check . --exclude "(venv|.venv|build|dist)"'. Exit code 1.
⏰ 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 (1)
cortex/cli.py (1)
1536-1543: Add type hints to method signature.The method signature lacks type hints for parameters and return type, which is required by the coding guidelines for Python code.
🔎 Add type hints
def _install_from_source( self, - package_name: str, - execute: bool, - dry_run: bool, - source_url: str | None, - version: str | None, + package_name: str, + execute: bool, + dry_run: bool, + source_url: str | None, + version: str | None, ) -> int:As per coding guidelines: Type hints required in Python code.
Likely an incorrect or invalid review comment.
| def _install_from_source( | ||
| self, | ||
| package_name: str, | ||
| execute: bool, | ||
| dry_run: bool, | ||
| source_url: str | None, | ||
| version: str | None, | ||
| ) -> int: | ||
| """Handle installation from source.""" | ||
| from cortex.source_builder import SourceBuilder | ||
|
|
||
| builder = SourceBuilder() | ||
|
|
||
| # 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 else None | ||
|
|
||
| cx_print(f"Building {package_name} from source...", "info") | ||
| if version: | ||
| cx_print(f"Version: {version}", "info") | ||
|
|
||
| result = builder.build_from_source( | ||
| package_name=package_name, | ||
| version=version, | ||
| source_url=source_url, | ||
| use_cache=True, | ||
| ) | ||
|
|
||
| if not result.success: | ||
| self._print_error(f"Build failed: {result.error_message}") | ||
| return 1 | ||
|
|
||
| if result.cached: | ||
| cx_print(f"Using cached build for {package_name}", "info") | ||
|
|
||
| if dry_run: | ||
| cx_print("\nBuild commands (dry run):", "info") | ||
| for cmd in result.install_commands: | ||
| console.print(f" • {cmd}") | ||
| return 0 | ||
|
|
||
| if not execute: | ||
| cx_print("\nBuild completed. Install commands:", "info") | ||
| for cmd in result.install_commands: | ||
| console.print(f" • {cmd}") | ||
| cx_print("Run with --execute to install", "info") | ||
| return 0 | ||
|
|
||
| # Execute install commands | ||
| from cortex.coordinator import InstallationCoordinator, InstallationStep, StepStatus | ||
|
|
||
| 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=result.install_commands, | ||
| descriptions=[f"Install {package_name}" for _ in result.install_commands], | ||
| timeout=600, | ||
| stop_on_error=True, | ||
| progress_callback=progress_callback, | ||
| ) | ||
|
|
||
| install_result = coordinator.execute() | ||
|
|
||
| if install_result.success: | ||
| self._print_success(f"{package_name} built and installed successfully!") | ||
| return 0 | ||
| else: | ||
| self._print_error("Installation failed") | ||
| if install_result.error_message: | ||
| console.print(f"Error: {install_result.error_message}", style="red") | ||
| return 1 |
There was a problem hiding this comment.
Implement audit logging for source-based installations.
The method bypasses the installation history tracking that is present in the regular install path (lines 579-616, 682-686, 743-746). Source-based installations should also be logged to ~/.cortex/history.db for consistency and traceability.
🔎 Add installation history tracking
def _install_from_source(
self,
package_name: str,
execute: bool,
dry_run: bool,
source_url: str | None,
version: str | None,
) -> int:
"""Handle installation from source."""
from cortex.source_builder import SourceBuilder
+ from datetime import datetime
builder = SourceBuilder()
+ history = InstallationHistory()
+ install_id = None
+ start_time = datetime.now()
# 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 else None
cx_print(f"Building {package_name} from source...", "info")
if version:
cx_print(f"Version: {version}", "info")
result = builder.build_from_source(
package_name=package_name,
version=version,
source_url=source_url,
use_cache=True,
)
if not result.success:
self._print_error(f"Build failed: {result.error_message}")
+ if install_id:
+ history.update_installation(install_id, InstallationStatus.FAILED, result.error_message)
return 1
if result.cached:
cx_print(f"Using cached build for {package_name}", "info")
if dry_run:
cx_print("\nBuild commands (dry run):", "info")
for cmd in result.install_commands:
console.print(f" • {cmd}")
+ if execute or dry_run:
+ install_id = history.record_installation(
+ InstallationType.INSTALL, [package_name], result.install_commands, start_time
+ )
+ history.update_installation(install_id, InstallationStatus.SUCCESS)
return 0
if not execute:
cx_print("\nBuild completed. Install commands:", "info")
for cmd in result.install_commands:
console.print(f" • {cmd}")
cx_print("Run with --execute to install", "info")
return 0
+ # Record installation start
+ install_id = history.record_installation(
+ InstallationType.INSTALL, [package_name], result.install_commands, start_time
+ )
+
# Execute install commands
from cortex.coordinator import InstallationCoordinator, InstallationStep, StepStatus
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=result.install_commands,
descriptions=[f"Install {package_name}" for _ in result.install_commands],
timeout=600,
stop_on_error=True,
progress_callback=progress_callback,
)
install_result = coordinator.execute()
if install_result.success:
self._print_success(f"{package_name} built and installed successfully!")
+ if install_id:
+ history.update_installation(install_id, InstallationStatus.SUCCESS)
+ print(f"\n📝 Installation recorded (ID: {install_id})")
+ print(f" To rollback: cortex rollback {install_id}")
return 0
else:
self._print_error("Installation failed")
+ if install_id:
+ error_msg = install_result.error_message or "Installation failed"
+ history.update_installation(install_id, InstallationStatus.FAILED, error_msg)
+ print(f"\n📝 Installation recorded (ID: {install_id})")
+ print(f" View details: cortex history {install_id}")
if install_result.error_message:
console.print(f"Error: {install_result.error_message}", style="red")
return 1Based on learnings: Implement audit logging to ~/.cortex/history.db for all package operations.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In cortex/cli.py around lines 1536-1614, the source-install path skips writing
to the ~/.cortex/history.db audit log; update the flow to record installation
history just like the regular install paths: after a build succeeds and before
returning success, call the same history persistence routine used elsewhere (or
import and use the HistoryDB/HistoryStore helper) to insert an entry with
package_name, version, source_url, method="source", timestamp, result="success";
likewise, when installation fails (install_result.success is False) record an
entry with result="failed" and include the error_message; ensure entries are
written for dry-run/cached cases as appropriate using the same schema and
storage location (~/.cortex/history.db) and place the calls immediately before
the early return statements so audit logging always runs.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
cortex/source_builder.py (3)
290-298: Verify that./configurescript is executable.Line 291 invokes
./configuredirectly, which will fail if the configure script doesn't have execute permissions. Some source archives ship configure without the executable bit set, requiringsh configureorbash configureinstead.🔎 Proposed fix
if config.build_system == "autotools": - configure_cmd = "./configure" + # Use sh to invoke configure in case executable bit is not set + configure_cmd = "sh ./configure" if config.configure_args: configure_cmd += " " + " ".join(config.configure_args)Alternatively, check and set executable permissions first:
if config.build_system == "autotools": + configure_script = source_dir / "configure" + if configure_script.exists(): + configure_script.chmod(configure_script.stat().st_mode | 0o111) configure_cmd = "./configure"
353-354: Python build command uses deprecatedsetup.pyinterface.Line 354 uses
python3 setup.py build, which has been deprecated in favor of PEP 517/518 build tools. Modern Python packages may not support directsetup.pyinvocation.🔎 Recommended alternatives
Replace with the modern build approach:
elif config.build_system == "python": - commands.append("python3 setup.py build") + # Use modern Python build tools (PEP 517) + commands.append("python3 -m pip install --user build && python3 -m build")Or for editable installs during development:
elif config.build_system == "python": - commands.append("python3 setup.py build") + commands.append("python3 -m pip wheel --no-deps -w dist .")
377-378: Python install command uses deprecatedsetup.pyinterface.Similar to the build command, line 378 uses the deprecated
python3 setup.py install. Modern Python packaging should use pip for installation.🔎 Recommended alternative
elif config.build_system == "python": - commands.append("sudo python3 setup.py install") + commands.append("sudo python3 -m pip install .")
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cortex/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.py
🧬 Code graph analysis (1)
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)
| 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] |
There was a problem hiding this comment.
CRITICAL: Cache key missing build configuration parameters.
The cache key is generated from only package_name, version, and source_url, but excludes configure_args and make_args. If a user builds the same package with different configuration options (e.g., different --prefix or optimization flags), they will incorrectly receive the cached build from the first configuration instead of a fresh build with the new options.
🔎 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]
+ def _get_cache_key(
+ self,
+ package_name: str,
+ version: str | None,
+ source_url: str,
+ configure_args: list[str] | None = None,
+ make_args: list[str] | None = None,
+ install_prefix: str = "/usr/local",
+ ) -> str:
+ """Generate a cache key for a build."""
+ # Include all configuration that affects build output
+ config_str = f"{configure_args or ''}:{make_args or ''}:{install_prefix}"
+ key_data = f"{package_name}:{version}:{source_url}:{config_str}"
+ return hashlib.sha256(key_data.encode()).hexdigest()[:16]Then update the call site at line 415 to pass these parameters:
- cache_key = self._get_cache_key(package_name, version, source_url)
+ cache_key = self._get_cache_key(
+ package_name, version, source_url, configure_args, make_args, install_prefix
+ )Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In cortex/source_builder.py around lines 98 to 101, the cache key only uses
package_name, version, and source_url and omits configure_args and make_args,
causing different build configurations to collide; update the _get_cache_key
function to include normalized representations of configure_args and make_args
(e.g., join lists into a stable string, sort or preserve order as appropriate,
and treat None as an empty string) when composing key_data before hashing, and
then update the call site at line 415 to pass the configure_args and make_args
parameters into _get_cache_key so the cache key reflects build configuration.
| def _save_to_cache(self, cache_key: str, build_dir: Path, install_commands: list[str]) -> None: | ||
| """Save build artifacts to cache.""" | ||
| 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() |
There was a problem hiding this comment.
MAJOR: Build cache stores only metadata, not artifacts.
The _save_to_cache method saves only metadata (build directory path, install commands, timestamp) but does not copy the actual build artifacts. Since build_dir points to a temporary directory created by tempfile.mkdtemp, the cached build becomes invalid once the temp directory is cleaned up by the system or manually removed.
🔎 Recommended approach
Either:
- Copy the built artifacts (compiled binaries, libraries) from
build_dirtocache_pathbefore saving metadata, or - Copy the installed files from
install_prefixto a staging area incache_path, or - Document that the cache only works for the current session and disable caching by default
Example for option 1:
def _save_to_cache(self, cache_key: str, build_dir: Path, install_commands: list[str]) -> None:
"""Save build artifacts to cache."""
cache_path = self.cache_dir / cache_key
cache_path.mkdir(parents=True, exist_ok=True)
+
+ # Copy build artifacts to cache
+ artifacts_dir = cache_path / "artifacts"
+ shutil.copytree(build_dir, artifacts_dir, dirs_exist_ok=True)
# Save metadata
metadata = {
- "build_dir": str(build_dir),
+ "build_dir": str(artifacts_dir),
"install_commands": install_commands,
"timestamp": str(Path(build_dir).stat().st_mtime),
}| def _fetch_from_url(self, url: str, package_name: str, version: str | None) -> Path: | ||
| """Fetch source from a URL.""" | ||
| temp_dir = Path(tempfile.mkdtemp(prefix=f"cortex-build-{package_name}-")) | ||
|
|
||
| try: | ||
| # Download | ||
| cx_print(f"📥 Downloading {package_name} source...", "info") | ||
| archive_path = temp_dir / "source.tar.gz" | ||
|
|
||
| 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" | ||
|
|
||
| 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"]: | ||
| with tarfile.open(archive_path, "r:gz") as tar: | ||
| tar.extractall(extract_dir) | ||
| elif archive_path.suffix == ".zip": | ||
| with zipfile.ZipFile(archive_path, "r") as zip_ref: | ||
| zip_ref.extractall(extract_dir) | ||
|
|
||
| # Find the actual source directory (usually one level deep) | ||
| extracted_items = list(extract_dir.iterdir()) | ||
| if len(extracted_items) == 1 and extracted_items[0].is_dir(): | ||
| return extracted_items[0] | ||
| else: | ||
| return extract_dir | ||
|
|
||
| except Exception as e: | ||
| logger.exception(f"Failed to fetch source from {url}") | ||
| raise RuntimeError(f"Failed to fetch source: {e}") |
There was a problem hiding this comment.
MAJOR: Temporary directory resource leak.
tempfile.mkdtemp at line 187 creates a temporary directory that is never explicitly cleaned up. The directory is returned to the caller and persists indefinitely, accumulating over multiple builds. The exception handler doesn't clean it up either.
🔎 Proposed fix
Consider one of these approaches:
Option 1: Store temp directories for cleanup later in the SourceBuilder class:
def __init__(self):
self.dependency_resolver = DependencyResolver()
self.cache_dir = CACHE_DIR
self.cache_dir.mkdir(parents=True, exist_ok=True)
+ self._temp_dirs = [] # Track temp dirs for cleanup
+
+ def cleanup(self):
+ """Clean up temporary directories."""
+ for temp_dir in self._temp_dirs:
+ if temp_dir.exists():
+ shutil.rmtree(temp_dir, ignore_errors=True)
+ self._temp_dirs.clear()Then in _fetch_from_url:
def _fetch_from_url(self, url: str, package_name: str, version: str | None) -> Path:
"""Fetch source from a URL."""
temp_dir = Path(tempfile.mkdtemp(prefix=f"cortex-build-{package_name}-"))
+ self._temp_dirs.append(temp_dir)Option 2: Use tempfile.TemporaryDirectory context manager at the build_from_source level and copy artifacts out before it's cleaned.
Option 3: Document that the build directory persists and provide a separate cleanup command.
Committable suggestion skipped: line range outside the PR's diff.
| else: | ||
| cmake_cmd += f" -DCMAKE_INSTALL_PREFIX={config.install_prefix}" | ||
| cmake_cmd += " .." | ||
| commands.append(f"cd {build_dir} && {cmake_cmd}") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check run_command implementation
rg -n "def run_command" -A 20 cortex/utils/commands.pyRepository: cortexlinux/cortex
Length of output: 1502
🏁 Script executed:
#!/bin/bash
# Check the lines in question in cortex/source_builder.py
sed -n '305,315p' cortex/source_builder.py
sed -n '347,357p' cortex/source_builder.py
sed -n '371,381p' cortex/source_builder.pyRepository: cortexlinux/cortex
Length of output: 1138
🏁 Script executed:
#!/bin/bash
# Check where the commands list is used in source_builder.py
rg -n "run_command" cortex/source_builder.py | head -20Repository: cortexlinux/cortex
Length of output: 389
🏁 Script executed:
#!/bin/bash
# Get broader context around line 309 to see the function and how commands are used
sed -n '280,330p' cortex/source_builder.pyRepository: cortexlinux/cortex
Length of output: 1804
🏁 Script executed:
#!/bin/bash
# Check the context around the run_command calls to understand which commands are being executed
sed -n '450,495p' cortex/source_builder.pyRepository: cortexlinux/cortex
Length of output: 2214
🏁 Script executed:
#!/bin/bash
# Search for calls to configure, build, install methods
rg -n "\.configure\(|\.build\(|\.install\(" cortex/source_builder.pyRepository: cortexlinux/cortex
Length of output: 125
🏁 Script executed:
#!/bin/bash
# Check the install method to see lines around 375 and how install commands are executed
sed -n '360,395p' cortex/source_builder.pyRepository: cortexlinux/cortex
Length of output: 1180
🏁 Script executed:
#!/bin/bash
# Check if there's an install phase that uses install_commands
rg -n "install_commands" cortex/source_builder.py -A 3 -B 3Repository: cortexlinux/cortex
Length of output: 3045
🏁 Script executed:
#!/bin/bash
# Check if install_commands are executed anywhere
rg -n "for.*install_commands\|install_commands\[" cortex/source_builder.pyRepository: cortexlinux/cortex
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Search across the whole repo for where install_commands results are used
rg -n "install_commands" --type py | grep -E "\.install_commands|in.*install_commands|for.*in"Repository: cortexlinux/cortex
Length of output: 845
🏁 Script executed:
#!/bin/bash
# Check how install_commands are executed in cli.py
sed -n '1570,1605p' cortex/cli.pyRepository: cortexlinux/cortex
Length of output: 1471
🏁 Script executed:
#!/bin/bash
# Check InstallationCoordinator to see how it executes commands
rg -n "class InstallationCoordinator" -A 50 cortex/coordinator.py | head -80Repository: cortexlinux/cortex
Length of output: 2047
🏁 Script executed:
#!/bin/bash
# Check the execute method of InstallationCoordinator
rg -n "def execute" cortex/coordinator.py -A 20Repository: cortexlinux/cortex
Length of output: 932
🏁 Script executed:
#!/bin/bash
# Check the _execute_command method
rg -n "def _execute_command" cortex/coordinator.py -A 15Repository: cortexlinux/cortex
Length of output: 672
🏁 Script executed:
#!/bin/bash
# Get more of the _execute_command method
sed -n '162,200p' cortex/coordinator.pyRepository: cortexlinux/cortex
Length of output: 1585
Fix compound shell commands in configure and build phases to use cwd parameter instead.
Lines 309 and 351 construct commands like cd {build_dir} && {cmake_cmd} and cd {build_dir} && {make_cmd} that are executed via run_command(cmd, cwd=str(source_dir), timeout=...). Since run_command defaults to use_shell=False, the shell operator && will not work and the command will fail because cd is a shell built-in.
Use the cwd parameter to set the working directory instead:
# Instead of: cd {build_dir} && {cmake_cmd}
result = run_command(cmake_cmd, cwd=str(build_dir), timeout=...)(Line 375 is not affected—install commands are executed via InstallationCoordinator, which uses shell=True.)
🤖 Prompt for AI Agents
cortex/source_builder.py around lines 309 and 351: the code currently builds
compound shell commands like "cd {build_dir} && {cmake_cmd}" and "cd {build_dir}
&& {make_cmd}" which will fail because run_command is invoked with
use_shell=False; remove the "cd ... &&" wrapper and call run_command with the
cwd parameter set to build_dir (e.g., run_command(cmake_cmd, cwd=str(build_dir),
timeout=...), and similarly for make_cmd), and ensure the command argument
format matches run_command's expectations (list form if use_shell=False) while
preserving timeout and other options.
| if cached_path: | ||
| cx_print(f"📦 Using cached build for {package_name}", "info") | ||
| metadata_path = cached_path / "metadata.json" | ||
| if metadata_path.exists(): | ||
| with open(metadata_path) as f: | ||
| metadata = json.load(f) | ||
| return BuildResult( | ||
| success=True, | ||
| package_name=package_name, | ||
| version=version, | ||
| build_dir=str(cached_path), | ||
| install_commands=metadata.get("install_commands", []), | ||
| cached=True, | ||
| ) |
There was a problem hiding this comment.
MAJOR: Cached builds return success without verifying installation.
When a cached build is found (lines 417-430), the method returns BuildResult with success=True and cached=True, but it does not execute the install_commands. This creates ambiguity:
- The package may not be installed on the system (install commands were never run)
- The "installed" marker in the cache (line 106) only indicates the build completed, not that the package is installed system-wide
- Users might assume the package is ready to use when it's not
🔎 Recommended approaches
Option 1: Execute install commands even for cached builds:
if cached_path:
cx_print(f"📦 Using cached build for {package_name}", "info")
metadata_path = cached_path / "metadata.json"
if metadata_path.exists():
with open(metadata_path) as f:
metadata = json.load(f)
+ # Execute install commands from cache
+ install_commands = metadata.get("install_commands", [])
+ for cmd in install_commands:
+ result = run_command(cmd, timeout=600)
+ if not result.success:
+ return BuildResult(
+ success=False,
+ package_name=package_name,
+ version=version,
+ build_dir=str(cached_path),
+ install_commands=install_commands,
+ error_message=f"Install failed: {result.stderr}",
+ )
return BuildResult(
success=True,Option 2: Check if the package is actually installed on the system before returning cached result:
if cached_path:
+ # Verify package is actually installed on the system
+ if not self.dependency_resolver.is_package_installed(package_name):
+ cx_print(f"📦 Cached build found but not installed, reinstalling...", "info")
+ # Fall through to normal build flow
+ else:
cx_print(f"📦 Using cached build for {package_name}", "info")Option 3: Document clearly in the docstring and return message that cached builds still require manual installation.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In cortex/source_builder.py around lines 417-430, the current early return for
cached builds marks success=True and cached=True without running
install_commands or verifying the package is installed; change this so that
after loading metadata from metadata.json you either (preferred) execute the
metadata.get("install_commands", []) and only return success=True if those
commands complete successfully (capture and log errors and return success=False
on failure), or (alternative) run a system-level check to verify the package is
actually installed before returning success; ensure cached=True is still set for
a cache hit but only mark success when installation has been validated or
commands ran successfully, and propagate any installation errors in the
BuildResult.
aybanda
force-pushed
the
feature/source-build-from-source
branch
from
ba60781 to
0889097
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
cortex/cli.py (2)
1549-1627: Missing audit logging for source-based installations.This method bypasses the installation history tracking present in the regular install path. Source-based installations should be logged to
~/.cortex/history.dbfor consistency and traceability, including:
- Recording installation start with
history.record_installation()- Updating status on success/failure with
history.update_installation()- Tracking build failures, dry-run executions, and successful installs
Based on learnings: Implement audit logging to ~/.cortex/history.db for all package operations.
1749-1763: Fix the--versionargument conflict.The global
--versionflag (line 1721) usesaction="version", which intercepts all--versioninvocations before subparser arguments are processed. Runningcortex install --version 3.12will print the cortex version and exit instead of passing the version to the source builder.Rename the install subparser's
--versionto--pkg-versionor--package-versionto avoid this conflict.🔎 Proposed fix
install_parser.add_argument( - "--version", + "--pkg-version", type=str, - help="Version to build (for --from-source)", + help="Package version to build (for --from-source)", )Then update line 2007:
from_source=getattr(args, "from_source", False), source_url=getattr(args, "source_url", None), - version=getattr(args, "version", None), + version=getattr(args, "pkg_version", None), )
🧹 Nitpick comments (2)
cortex/cli.py (2)
1549-1558: Expand docstring to meet PEP 257 requirements.The docstring should document parameters, return values, and potential exceptions. Consider this format:
def _install_from_source( self, package_name: str, execute: bool, dry_run: bool, source_url: str | None, version: str | None, ) -> int: - """Handle installation from source.""" + """Handle installation from source. + + Args: + package_name: Name of package to build (may include @version suffix) + execute: Whether to execute installation commands + dry_run: Whether to show commands without building/installing + source_url: Optional URL to source code tarball or repository + version: Optional version to build (overrides @version in package_name) + + Returns: + Exit code (0 for success, 1 for failure) + """ from cortex.source_builder import SourceBuilderAs per coding guidelines: Docstrings required for all public APIs.
1613-1613: Consider making the timeout configurable.The 600-second (10-minute) timeout is hardcoded for source builds. Complex builds (e.g., large C++ projects, LLVM) may require longer timeouts. Consider either:
- Adding a
--timeoutCLI argument for source builds- Using a larger default timeout for source builds (e.g., 1800s/30min)
- Documenting the timeout limitation in SOURCE_BUILD.md
📜 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
🪛 GitHub Actions: CI
cortex/cli.py
[error] 1-1: Black formatting check failed. 1 file would be reformatted. Command failed: 'black --check . --exclude "(venv|.venv|build|dist)"'. Please run 'black .' to format the code.
⏰ 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 (2)
cortex/cli.py (2)
560-562: LGTM: Parameter additions are well-typed.The new parameters for source-based installation are correctly typed with appropriate defaults and follow the Python 3.10+ union syntax used throughout the codebase.
597-601: LGTM: Clean separation of source-based installation path.The early return appropriately separates the source-build flow from the LLM-based installation path, forwarding all necessary parameters.
| f"{r.id:<18} {date:<20} {r.operation_type.value:<12} " | ||
| f"{packages:<30} {r.status.value:<15}" |
There was a problem hiding this comment.
Apply Black formatting to resolve pipeline failure.
The CI pipeline reports a Black formatting check failure. Run black . to auto-format the code before merging.
#!/bin/bash
# Verify Black formatting issues
black --check cortex/cli.py --diff🤖 Prompt for AI Agents
In cortex/cli.py around lines 891-892, the CI failed Black formatting on the
f-string lines; run Black to reformat the file and commit the changes (e.g., run
`black .` or `black cortex/cli.py`), then re-run the formatting check to ensure
the f-string line breaks and spacing conform to Black before pushing the updated
file.
| from_source=getattr(args, "from_source", False), | ||
| source_url=getattr(args, "source_url", None), | ||
| version=getattr(args, "version", None), |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Update parameter name when --version is renamed.
Once the install subparser's --version argument is renamed to --pkg-version (see comment on lines 1749-1763), update line 2007 accordingly:
from_source=getattr(args, "from_source", False),
source_url=getattr(args, "source_url", None),
- version=getattr(args, "version", None),
+ version=getattr(args, "pkg_version", None),
)📝 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.
| from_source=getattr(args, "from_source", False), | |
| source_url=getattr(args, "source_url", None), | |
| version=getattr(args, "version", None), | |
| from_source=getattr(args, "from_source", False), | |
| source_url=getattr(args, "source_url", None), | |
| version=getattr(args, "pkg_version", None), |
🤖 Prompt for AI Agents
In cortex/cli.py around lines 2005 to 2007, the call is still using
getattr(args, "version", None) but the install subparser renamed the CLI flag to
--pkg-version; update this line to use getattr(args, "pkg_version", None) (and
keep from_source and source_url as-is) so the code reads the new parameter name
from args.
|
2 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
Changes Made
Core Implementation
cortex/source_builder.py- Core source building module (518 lines)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
Checklist
Summary by CodeRabbit
New Features
Documentation
Bug Fixes / UX
Tests
✏️ Tip: You can customize this high-level summary in your review settings.