add aybanda CLA Signature #457
Conversation
- 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
📝 WalkthroughWalkthroughThis PR introduces source-build capabilities to Cortex CLI, enabling users to build and install packages from source code instead of pre-built binaries. A new SourceBuilder module orchestrates source fetching, build-system detection, dependency resolution, configuration, compilation, caching, and installation. CLI support includes Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI as CortexCLI
participant Builder as SourceBuilder
participant Resolver as DependencyResolver
participant Coordinator as InstallationCoordinator
participant FileSystem as FileSystem/<br/>Package Source
User->>CLI: install --from-source
activate CLI
CLI->>Builder: build_from_source()
activate Builder
Builder->>FileSystem: fetch_source()
activate FileSystem
FileSystem-->>Builder: source_path
deactivate FileSystem
Builder->>Builder: detect_build_system()
Note over Builder: Identify autotools/<br/>cmake/make/python
Builder->>Resolver: detect_build_dependencies()
activate Resolver
Resolver-->>Builder: required_deps[]
deactivate Resolver
alt dependencies_missing
Builder->>FileSystem: apt-get install
activate FileSystem
FileSystem-->>Builder: installed
deactivate FileSystem
end
Builder->>Builder: configure_build()
Note over Builder: Generate config<br/>commands
Builder->>Builder: build()
Note over Builder: Execute make<br/>(with parallelization)
Builder->>Builder: install_build()
Note over Builder: Generate install<br/>commands
Builder->>Coordinator: execute install commands<br/>with progress callback
activate Coordinator
Coordinator->>FileSystem: execute commands
activate FileSystem
FileSystem-->>Coordinator: success
deactivate FileSystem
Coordinator-->>Builder: result
deactivate Coordinator
Builder->>Builder: _save_to_cache()
Note over Builder: Cache build artifacts<br/>and metadata
Builder-->>CLI: BuildResult
deactivate Builder
CLI->>User: install complete
deactivate CLI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (4 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
CLA Verification FailedThe following contributors have not signed the Contributor License Agreement:
How to Sign
This check runs automatically. Maintainers can update |
|
There was a problem hiding this comment.
@aybanda . Try to update branch and feel free to open a new PR.
There was a problem hiding this comment.
This files are irrelevant, why changed ?
![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 (8)
docs/SOURCE_BUILD.md (1)
78-82: Consider updating Python build documentation for modern tooling.The documentation mentions
python3 setup.py build/install, which is deprecated. Modern Python packaging recommends usingpip install .orpython -m build. Consider adding a note about this or updating the implementation to supportpyproject.toml-based builds withpip.cortex/cli.py (3)
546-549: Add return type annotation to method signature.The
installmethod signature was updated with new parameters but is missing the return type annotation. As per coding guidelines, type hints are required in Python code.🔎 Proposed fix
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:
1547-1550: Edge case: Multiple@symbols in package name.The version extraction logic splits on
@and takes the second element, but this could produce unexpected results if the package name contains multiple@symbols (e.g., scoped npm-style packages). Consider usingrsplitwithmaxsplit=1to split from the right.🔎 Proposed fix
# Parse version from package name if specified (e.g., python@3.12) if "@" in package_name and not version: - parts = package_name.split("@") + parts = package_name.rsplit("@", 1) package_name = parts[0] version = parts[1] if len(parts) > 1 else None
1584-1584: Remove redundant import.
InstallationCoordinator,InstallationStep, andStepStatusare already imported at line 11. This inline import is unnecessary.🔎 Proposed fix
# Execute install commands - from cortex.coordinator import InstallationCoordinator, InstallationStep, StepStatus - def progress_callback(current: int, total: int, step: InstallationStep) -> None:cortex/source_builder.py (2)
32-34: Avoid module-level side effects.Creating the cache directory at module import time can cause issues in testing and when the module is imported but not used. Consider lazy initialization in
__init__instead.🔎 Proposed fix
# Build cache directory CACHE_DIR = Path.home() / ".cortex" / "build_cache" -CACHE_DIR.mkdir(parents=True, exist_ok=True)The directory is already created in
__init__at line 93, so this line is redundant and causes the side effect.
265-268: Questionable default: Defaulting to autotools when no build system detected.When no build files are found, defaulting to
autotoolsmay lead to confusing errors. Consider returning"unknown"or raising an exception to prompt the user for explicit configuration.🔎 Proposed fix
# Check for Python setup.py or pyproject.toml if (source_dir / "setup.py").exists() or (source_dir / "pyproject.toml").exists(): return "python" - # Default to autotools (most common) - return "autotools" + # No recognized build system found + return "unknown"Then handle
"unknown"inbuild_from_sourceto provide a helpful error message.tests/test_source_builder.py (2)
8-8: Remove unused import.The
osmodule is imported but not used in this test file.🔎 Proposed fix
-import os import tempfile
235-253: Strengthen success path assertions.The test only verifies
result is not None, which is a weak assertion. Consider also checkingresult.successto ensure the build actually succeeded.🔎 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 all dependencies are satisfied and commands succeed + assert result is not None + assert result.success is True + assert result.package_name == "test-package"
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/cla-signers.jsoncortex/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/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
🧬 Code graph analysis (1)
cortex/source_builder.py (3)
cortex/branding.py (1)
cx_print(49-69)cortex/dependency_resolver.py (1)
DependencyResolver(40-386)cortex/utils/commands.py (1)
CommandResult(118-125)
🪛 GitHub Actions: CI
cortex/source_builder.py
[error] 202-202: Ruff check failed. 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). (4)
- GitHub Check: test (3.11)
- GitHub Check: test (3.10)
- GitHub Check: test (3.12)
- GitHub Check: Analyze Python
🔇 Additional comments (7)
.github/cla-signers.json (1)
126-134: LGTM!The CLA signer entry for Ajay Bandaru (aybanda) is correctly formatted with all required fields matching the details from issue #456.
cortex/cli.py (1)
1533-1611: LGTM - Well-structured source installation handler.The
_install_from_sourcemethod is well-implemented with:
- Proper type hints and docstring
- Clear separation of dry-run, preview, and execute modes
- Appropriate progress feedback via callbacks
- Consistent error handling pattern
cortex/source_builder.py (2)
50-76: LGTM - Well-designed dataclasses.
BuildConfigandBuildResultdataclasses are well-structured with appropriate defaults and clear field names. Good use ofstr | Noneunion types.
78-93: LGTM - SourceBuilder initialization.The class has a clear docstring describing its purpose and attributes. Initialization properly sets up the dependency resolver and cache directory.
tests/test_source_builder.py (3)
211-233: LGTM - Good failure path testing.This test properly validates the error handling when dependency installation fails, including checking the error message content.
23-46: LGTM - Good initialization and cache key tests.The tests properly verify SourceBuilder initialization and cache key determinism/uniqueness. The 16-character length assertion for cache keys is a good implementation detail check.
84-117: LGTM - Comprehensive build system detection tests.Good coverage of all supported build systems (autotools, cmake, make, python) plus the default case. Using
tempfile.TemporaryDirectoryis the correct approach for filesystem-based tests.
| urllib.request.urlretrieve(url, archive_path) | ||
|
|
||
| # Extract | ||
| cx_print(f"📦 Extracting source...", "info") |
There was a problem hiding this comment.
Fix f-string without placeholders (lint failure).
This f-string has no placeholders, causing the pipeline to fail. Remove the f prefix.
🔎 Proposed fix
- cx_print(f"📦 Extracting source...", "info")
+ cx_print("📦 Extracting source...", "info")📝 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.
| cx_print(f"📦 Extracting source...", "info") | |
| cx_print("📦 Extracting source...", "info") |
🧰 Tools
🪛 GitHub Actions: CI
[error] 202-202: Ruff check failed. F541 f-string without any placeholders.
🪛 GitHub Check: lint
[failure] 202-202: Ruff (F541)
cortex/source_builder.py:202:22: F541 f-string without any placeholders
🪛 GitHub Check: Lint
[failure] 202-202: Ruff (F541)
cortex/source_builder.py:202:22: F541 f-string without any placeholders
🤖 Prompt for AI Agents
In cortex/source_builder.py around line 202, the call cx_print(f"📦 Extracting
source...", "info") uses an unnecessary f-string prefix with no placeholders;
remove the leading 'f' so the string is a normal literal (cx_print("📦
Extracting source...", "info")) to resolve the lint failure.
| 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.
🧩 Analysis chain
🌐 Web query:
Python tarfile extractall filter parameter security DeprecationWarning Python 3.12
💡 Result:
Short answer
- Python 3.12 added a filter argument to TarFile.extract() / extractall() and an extraction_filter attribute. If you call extractall() without a filter in 3.12/3.13 you get a DeprecationWarning and it falls back to the old (unsafe) behavior. In 3.14 the default will become the safer "data" filter. [1][2][3][4]
What to do
- Explicitly pass a safe filter when extracting untrusted archives, e.g. filter="data" (recommended) or provide a custom callable. Or set TarFile.extraction_filter on your TarFile (or TarFile class) if you want a global default.
Minimal example
- Safe extraction using the built-in data filter:
import tarfile
with tarfile.open("archive.tar") as tf:
tf.extractall(path="dest", filter="data")
References
- tarfile docs (3.12): extraction filters and filter argument. [1]
- PEP 706 (feature + deprecation schedule). [2]
- What's New in Python 3.12 (notes about tarfile deprecation). [3]
- tarfile docs (3.14) showing default becomes "data". [4]
🏁 Script executed:
# Find and examine the actual code file
fd -t f "source_builder.py" cortex/ --exec head -n 250 {}Repository: cortexlinux/cortex
Length of output: 8867
🏁 Script executed:
# Check for Python version requirements in project config
fd -t f -E ".*" "pyproject.toml|setup.py|setup.cfg|requirements.txt" --max-results 5 --exec head -n 50 {}Repository: cortexlinux/cortex
Length of output: 311
🏁 Script executed:
# Find project configuration files to check Python version requirements
find . -type f \( -name "pyproject.toml" -o -name "setup.py" -o -name "setup.cfg" -o -name ".python-version" -o -name "README.md" \) | head -20Repository: cortexlinux/cortex
Length of output: 136
🏁 Script executed:
# Look for Python version constraints in common places
cat pyproject.toml 2>/dev/null | head -100 || echo "No pyproject.toml"Repository: cortexlinux/cortex
Length of output: 2573
🏁 Script executed:
# Check the actual problematic code section more carefully
sed -n '200,220p' cortex/source_builder.pyRepository: cortexlinux/cortex
Length of output: 929
🌐 Web query:
Python 3.11 tarfile filter parameter extractall
💡 Result:
Short answer
- Python 3.11 (specifically 3.11.4+) added a filter keyword to TarFile.extract() / extractall(). The filter can be a string name (e.g. 'fully_trusted', 'tar', 'data') or a callable filter(member: TarInfo, path: str) -> TarInfo | None. The callable is called for each member and may return:
- a TarInfo to use (possibly modified),
- None to skip the member, or
- raise an exception (behavior depends on TarFile.errorlevel). [1][2]
Example (callable that skips any member outside target directory):
from tarfile import open as taropen
def safe_filter(member, path):
if '..' in member.name.split('/'):
return None
return member
with taropen('archive.tar') as tf:
tf.extractall(path='outdir', filter=safe_filter)
Notes
- Default filters and the TarFile.extraction_filter attribute were introduced to control default behavior; see PEP 706 and the Python docs for security recommendations (use 'data' to avoid many dangerous cases). [1][2]
Sources
[1] Python 3.11 tarfile documentation (extractall / extraction filters).
[2] PEP 706 — Filter for tarfile.extractall.
Security: Handle tarfile extraction safely across Python versions.
tarfile.extractall() without a filter is vulnerable to path traversal attacks (CVE-2007-4559). The filter parameter was added in Python 3.11.4+. Since the project supports Python >= 3.10, the proposed fix using filter='data' will break compatibility with Python 3.10 and Python 3.11.0-3.11.3.
Use a version-aware approach instead:
import sys
if archive_path.suffix == ".gz" or archive_path.suffixes[-2:] == [".tar", ".gz"]:
with tarfile.open(archive_path, "r:gz") as tar:
if sys.version_info >= (3, 11, 4):
tar.extractall(extract_dir, filter='data')
else:
# For older Python versions, use custom filter or consider upgrading
tar.extractall(extract_dir)Alternatively, drop Python 3.10 support and use filter='data' unconditionally if possible.
| ) | ||
|
|
||
| # Detect and install build dependencies | ||
| cx_print(f"🔍 Checking build dependencies...", "info") |
There was a problem hiding this comment.
Fix f-string without placeholders (lint failure).
This f-string has no placeholders. Remove the f prefix.
🔎 Proposed fix
- cx_print(f"🔍 Checking build dependencies...", "info")
+ cx_print("🔍 Checking build dependencies...", "info")📝 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.
| cx_print(f"🔍 Checking build dependencies...", "info") | |
| cx_print("🔍 Checking build dependencies...", "info") |
🧰 Tools
🪛 GitHub Check: lint
[failure] 444-444: Ruff (F541)
cortex/source_builder.py:444:22: F541 f-string without any placeholders
🪛 GitHub Check: Lint
[failure] 444-444: Ruff (F541)
cortex/source_builder.py:444:22: F541 f-string without any placeholders
🤖 Prompt for AI Agents
In cortex/source_builder.py around line 444, the string call uses an f-string
with no placeholders (cx_print(f"🔍 Checking build dependencies...", "info")),
causing a lint failure; remove the unnecessary leading "f" so the literal string
is passed (cx_print("🔍 Checking build dependencies...", "info")) and run
linters to verify.
| error_message=f"Failed to install build dependencies: {result.stderr}", | ||
| ) | ||
| else: | ||
| cx_print(f" ✓ All build dependencies satisfied", "success") |
There was a problem hiding this comment.
Fix f-string without placeholders (lint failure).
This f-string has no placeholders. Remove the f prefix.
🔎 Proposed fix
- cx_print(f" ✓ All build dependencies satisfied", "success")
+ cx_print(" ✓ All build dependencies satisfied", "success")🧰 Tools
🪛 GitHub Check: lint
[failure] 461-461: Ruff (F541)
cortex/source_builder.py:461:26: F541 f-string without any placeholders
🪛 GitHub Check: Lint
[failure] 461-461: Ruff (F541)
cortex/source_builder.py:461:26: F541 f-string without any placeholders
🤖 Prompt for AI Agents
In cortex/source_builder.py around line 461, the string literal uses an
unnecessary f-string prefix; remove the leading "f" so the line becomes a plain
string literal (i.e., change cx_print(f" ✓ All build dependencies satisfied",
"success") to cx_print(" ✓ All build dependencies satisfied", "success")).
Ensure no other f-strings without placeholders exist nearby.
| cx_print(f" ✓ All build dependencies satisfied", "success") | ||
|
|
||
| # Configure | ||
| cx_print(f"⚙️ Configuring build...", "info") |
There was a problem hiding this comment.
Fix f-string without placeholders (lint failure).
This f-string has no placeholders. Remove the f prefix.
🔎 Proposed fix
- cx_print(f"⚙️ Configuring build...", "info")
+ cx_print("⚙️ Configuring build...", "info")📝 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.
| cx_print(f"⚙️ Configuring build...", "info") | |
| cx_print("⚙️ Configuring build...", "info") |
🧰 Tools
🪛 GitHub Check: lint
[failure] 464-464: Ruff (F541)
cortex/source_builder.py:464:22: F541 f-string without any placeholders
🪛 GitHub Check: Lint
[failure] 464-464: Ruff (F541)
cortex/source_builder.py:464:22: F541 f-string without any placeholders
🤖 Prompt for AI Agents
In cortex/source_builder.py around line 464, the f-string f"⚙️ Configuring
build..." has no placeholders; remove the unnecessary f prefix so the string is
a regular literal (e.g., "⚙️ Configuring build...") to fix the lint failure.
| ) | ||
|
|
||
| # Build | ||
| cx_print(f"🔨 Compiling (this may take a while)...", "info") |
There was a problem hiding this comment.
Fix f-string without placeholders (lint failure).
This f-string has no placeholders. Remove the f prefix.
🔎 Proposed fix
- cx_print(f"🔨 Compiling (this may take a while)...", "info")
+ cx_print("🔨 Compiling (this may take a while)...", "info")📝 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.
| cx_print(f"🔨 Compiling (this may take a while)...", "info") | |
| cx_print("🔨 Compiling (this may take a while)...", "info") |
🧰 Tools
🪛 GitHub Check: lint
[failure] 479-479: Ruff (F541)
cortex/source_builder.py:479:22: F541 f-string without any placeholders
🪛 GitHub Check: Lint
[failure] 479-479: Ruff (F541)
cortex/source_builder.py:479:22: F541 f-string without any placeholders
🤖 Prompt for AI Agents
In cortex/source_builder.py around line 479, the code uses an f-string with no
placeholders (f"🔨 Compiling (this may take a while)..."), causing a lint
failure; remove the unnecessary f prefix so the string is a normal literal
(i.e., replace f"..." with "..." ) to satisfy the linter.
| ## 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
# Search for the three referenced documentation files
echo "=== Searching for Getting-Started.md ==="
fd -t f "Getting-Started.md"
echo -e "\n=== Searching for README_DEPENDENCIES.md ==="
fd -t f "README_DEPENDENCIES.md"
echo -e "\n=== Searching for COMMANDS.md ==="
fd -t f "COMMANDS.md"
# Also list all markdown files in docs/ to see what's available
echo -e "\n=== All markdown files in docs/ directory ==="
fd -t f "\.md$" docs/ | head -20Repository: cortexlinux/cortex
Length of output: 834
🏁 Script executed:
#!/bin/bash
# Find SOURCE_BUILD.md
fd -t f "SOURCE_BUILD.md"
# Check the actual "See Also" section in the file
echo -e "\n=== Content around line 237-242 ==="
fd -t f "SOURCE_BUILD.md" --exec cat -n {} \; | sed -n '235,245p'Repository: cortexlinux/cortex
Length of output: 347
Fix broken documentation links in "See Also" section.
Two of the three referenced files use incorrect relative paths and will not resolve:
Getting-Started.mdshould beguides/Getting-Started.mdREADME_DEPENDENCIES.mdshould bemodules/README_DEPENDENCIES.mdCOMMANDS.mdis correct (no change needed)
Update the links to include the proper subdirectory paths.
🤖 Prompt for AI Agents
In docs/SOURCE_BUILD.md around lines 237 to 242, the "See Also" section contains
two broken relative links; update `Getting-Started.md` to
`guides/Getting-Started.md` and `README_DEPENDENCIES.md` to
`modules/README_DEPENDENCIES.md` while leaving `COMMANDS.md` unchanged so the
three links resolve correctly.
2 participants
Related Issue
Closes #456
Summary
Checklist
pytest tests/)Summary by CodeRabbit
New Features
--from-source,--source-url, and--versionCLI flagsDocumentation
✏️ Tip: You can customize this high-level summary in your review settings.