Skip to content

Conversation

@Anshgrover23
Copy link
Collaborator

@Anshgrover23 Anshgrover23 commented Dec 30, 2025

Related Issue

Closes #130

Summary

Implements Docker-based package sandbox testing as requested in #130. Test packages in isolated containers before installing to the main system.

Acceptance Criteria

  • Create isolated environments
  • Install packages in sandbox
  • Run automated tests
  • Functional validation
  • Safe promotion to prod
  • Automatic cleanup
  • Unit tests included (>80% coverage)
  • Documentation with sandbox workflow

Demo

Screenshots

Screenshot From 2025-12-30 13-33-02 Screenshot From 2025-12-30 13-33-41 Screenshot From 2025-12-30 13-33-59 Screenshot From 2025-12-30 15-24-24 Screenshot From 2025-12-30 13-44-39

Video

Untitled design

Checklist

  • Tests pass (pytest tests/)
  • MVP label added if closing MVP issue
  • Update "Cortex -h" (if needed)

Summary by CodeRabbit

  • New Features
    • Added cortex sandbox command with subcommands (create, install, test, promote, cleanup, list, exec) for Docker-based isolated package testing, dry-run promotions, and in-sandbox command execution.
  • Documentation
    • Added command reference entry and detailed sandbox docs (usage, examples, troubleshooting, requirements, storage).
  • API / Packaging
    • Exposed sandbox public API surface for programmatic use.
  • Tests
    • Added comprehensive tests for sandbox lifecycle, install/test/promote flows, execution, compatibility, and metadata.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 30, 2025

📝 Walkthrough

Walkthrough

Adds a Docker-backed sandbox subsystem and CLI wiring to create, install, test, promote, exec, list, and clean isolated package test environments; includes implementation, public API exports, CLI help, documentation, and unit tests.

Changes

Cohort / File(s) Summary
CLI Integration
cortex/cli.py
Adds top-level sandbox command and dispatcher; implements CortexCLI.sandbox and private handlers (_sandbox_create, _sandbox_install, _sandbox_test, _sandbox_promote, _sandbox_cleanup, _sandbox_list, _sandbox_exec); updates rich help and main() dispatch.
Sandbox Public API
cortex/sandbox/__init__.py
Exposes sandbox public surface: DockerNotFoundError, DockerSandbox, SandboxAlreadyExistsError, SandboxExecutionResult, SandboxInfo, SandboxNotFoundError, SandboxState, SandboxTestResult, SandboxTestStatus, docker_available, CommandBlocked, ExecutionResult, SandboxExecutor, etc.
Docker sandbox implementation
cortex/sandbox/docker_sandbox.py
New DockerSandbox: enums/dataclasses (SandboxState, SandboxTestStatus, SandboxInfo, SandboxTestResult, SandboxExecutionResult), exceptions, docker availability checks, lifecycle (create/list/get/cleanup), package install, test, promote, exec_command, blocked-command filtering, metadata persistence, and helpers.
Docs: commands & guide
README.md, docs/COMMANDS.md, docs/SANDBOX.md
Adds README command reference entry for cortex sandbox; detailed COMMANDS.md usage and new SANDBOX.md guide covering workflow, examples, storage, limitations, troubleshooting.
Tests
tests/test_docker_sandbox.py
New comprehensive test suite covering Docker detection, lifecycle (create/install/test/promote/cleanup), exec and blocked commands, compatibility checks, metadata serialization, and docker_available(), using mocks/patches and temp metadata.
CI script formatting
.github/scripts/cla_check.py
Formatting and signer-data-shape updates in CLA check script (signature/structure formatting changes, richer author data handling).

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant User
    participant CLI as CortexCLI
    participant Sandbox as DockerSandbox
    participant Docker as Docker Engine
    participant Meta as Metadata Store

    rect rgba(0,128,96,0.06)
    Note over CLI,Sandbox: Create sandbox
    User->>CLI: cortex sandbox create <name>
    CLI->>Sandbox: create(name, image?)
    Sandbox->>Docker: check & pull/run container
    Docker-->>Sandbox: container created
    Sandbox->>Meta: save SandboxInfo
    Sandbox-->>CLI: SandboxExecutionResult(success)
    CLI-->>User: success
    end

    rect rgba(0,96,192,0.06)
    Note over CLI,Sandbox: Install & Test
    User->>CLI: cortex sandbox install <name> <pkg>
    CLI->>Sandbox: install(name, package)
    Sandbox->>Docker: apt-get install in container
    Docker-->>Sandbox: stdout/stderr/exit
    Sandbox->>Meta: update packages_installed
    Sandbox-->>CLI: SandboxExecutionResult(success)

    User->>CLI: cortex sandbox test <name> [pkg...]
    CLI->>Sandbox: test(name, package?)
    Sandbox->>Docker: run validation commands
    Docker-->>Sandbox: per-package test results
    Sandbox-->>CLI: SandboxExecutionResult(test_results)
    CLI-->>User: report
    end

    rect rgba(192,96,0,0.06)
    Note over CLI,Sandbox: Promote & Cleanup
    User->>CLI: cortex sandbox promote <name> <pkg> [--dry-run]
    CLI->>Sandbox: promote(name, package, dry_run)
    Sandbox->>Docker: verify package present
    Sandbox-->>CLI: prompt/confirm
    Sandbox->>Docker: (host) apt-get install (or dry-run)
    Sandbox-->>CLI: SandboxExecutionResult(success)

    User->>CLI: cortex sandbox cleanup <name>
    CLI->>Sandbox: cleanup(name, force?)
    Sandbox->>Docker: stop & remove container
    Sandbox->>Meta: delete SandboxInfo
    Sandbox-->>CLI: SandboxExecutionResult(success)
    CLI-->>User: success
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

  • cortexlinux/cortex#392 — Also adds new top-level CLI commands and modifies cortex/cli.py (help table and dispatch), touching similar CLI wiring.

Suggested labels

MVP

Suggested reviewers

  • Suyashd999
  • mikejmorgan-ai

Poem

🐇 I dug a tiny Docker den,
I spun up shells and tested then,
Packages tried in a cozy tub,
Bunny nods — promote with stub,
Clean sweep, happy hops, and a carrot rub! 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Out of Scope Changes check ❓ Inconclusive Most changes align with issue #130 objectives; however, modifications to .github/scripts/cla_check.py appear unrelated to sandbox testing and warrant clarification on their necessity. Verify whether CLA check script changes are essential for this PR or should be separated into a distinct pull request.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The PR title '[sandbox] Add Docker-based package sandbox testing environment' clearly and concisely describes the main change: introducing a Docker-based sandbox feature for testing packages.
Description check ✅ Passed The PR description includes all required template sections: Related Issue (#130), Summary explaining the feature, Acceptance Criteria with checkmarks, and most of the Checklist items marked complete (only MVP label unchecked, which is optional).
Linked Issues check ✅ Passed The PR implementation meets all coding requirements from issue #130: isolated Docker environments, package installation, automated testing with functional validation, safe promotion, automatic cleanup, comprehensive unit tests, and documentation.
Docstring Coverage ✅ Passed Docstring coverage is 91.03% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link

github-actions bot commented Dec 30, 2025

CLA Verification Passed

All contributors have signed the CLA.

Contributor Signed As
@Anshgrover23 @Anshgrover23
@Anshgrover23 @Anshgrover23

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (5)
docs/SANDBOX.md (1)

158-170: Add language specifiers to fenced code blocks.

Static analysis flagged missing language specifiers on code blocks. Consider using text or console for output examples:

🔎 Proposed fix
-```
+```text
 # Output:
 #    ✓  docker.io: package installed
 #    ✓  docker.io: no conflicts
-```
+```

Apply similar fixes to other output blocks (around lines 169 and 215).

cortex/cli.py (3)

289-298: Redundant import of SandboxTestStatus.

SandboxTestStatus is imported on line 297 within the sandbox() method, and again on line 391 within _sandbox_test(). Consider importing it once at the method level or moving all sandbox imports to the top of the sandbox() method.

🔎 Proposed fix

Remove the duplicate import on line 391:

     def _sandbox_test(self, sandbox, args: argparse.Namespace) -> int:
         """Run tests in sandbox."""
-        from cortex.sandbox import SandboxTestStatus
-
         name = args.name

The SandboxTestStatus from the parent sandbox() method import is already in scope.


427-430: Unused result in dry-run path.

The result from sandbox.promote(name, package, dry_run=True) on line 428 is captured but never used. The method immediately prints a hardcoded message instead.

🔎 Proposed fix

Either use the result message or remove the unnecessary call:

         if dry_run:
-            result = sandbox.promote(name, package, dry_run=True)
-            cx_print(f"Would run: sudo apt-get install -y {package}", "info")
+            cx_print(f"Would run: sudo apt-get install -y {package}", "info")
             return 0

Alternatively, use result.message if it contains more detailed information.


354-357: Add type hints for the sandbox parameter.

Per coding guidelines, type hints are required. The private sandbox action handlers are missing type annotations for the sandbox parameter.

🔎 Proposed fix
+from cortex.sandbox import DockerSandbox  # At top of sandbox() or module level
+
-    def _sandbox_create(self, sandbox, args: argparse.Namespace) -> int:
+    def _sandbox_create(self, sandbox: DockerSandbox, args: argparse.Namespace) -> int:
         """Create a new sandbox environment."""

Apply similar type hints to all private sandbox methods: _sandbox_install, _sandbox_test, _sandbox_promote, _sandbox_cleanup, _sandbox_list, _sandbox_exec.

Also applies to: 372-375, 389-394, 420-425, 457-460, 472-474, 493-496

cortex/sandbox/docker_sandbox.py (1)

326-350: Consider validating sandbox name to prevent edge cases.

The sandbox name is used directly in container naming and filesystem paths without validation. Malformed names (e.g., containing /, .., or shell metacharacters) could cause unexpected behavior.

🔎 Proposed fix

Add name validation at the start of create():

def create(
    self,
    name: str,
    image: str | None = None,
) -> SandboxExecutionResult:
    # Validate sandbox name
    if not name or not name.replace("-", "").replace("_", "").isalnum():
        return SandboxExecutionResult(
            success=False,
            message="Sandbox name must contain only alphanumeric characters, hyphens, and underscores",
            exit_code=1,
        )
    
    self.require_docker()
    # ... rest of method
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7894bdc and 992de3e.

📒 Files selected for processing (7)
  • README.md
  • cortex/cli.py
  • cortex/sandbox/__init__.py
  • cortex/sandbox/docker_sandbox.py
  • docs/COMMANDS.md
  • docs/SANDBOX.md
  • tests/test_docker_sandbox.py
🧰 Additional context used
📓 Path-based instructions (4)
**/*.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_docker_sandbox.py
  • cortex/sandbox/docker_sandbox.py
  • cortex/sandbox/__init__.py
  • cortex/cli.py
tests/**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

Maintain >80% test coverage for pull requests

Files:

  • tests/test_docker_sandbox.py
**/*sandbox*.py

📄 CodeRabbit inference engine (AGENTS.md)

Implement Firejail sandboxing for package operations

Files:

  • tests/test_docker_sandbox.py
  • cortex/sandbox/docker_sandbox.py
{setup.py,setup.cfg,pyproject.toml,**/__init__.py}

📄 CodeRabbit inference engine (AGENTS.md)

Use Python 3.10 or higher as the minimum supported version

Files:

  • cortex/sandbox/__init__.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T12:03:24.071Z
Learning: Applies to **/*sandbox*.py : Implement Firejail sandboxing for package operations
📚 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 **/*sandbox*.py : Implement Firejail sandboxing for package operations

Applied to files:

  • docs/SANDBOX.md
  • tests/test_docker_sandbox.py
  • docs/COMMANDS.md
  • cortex/sandbox/docker_sandbox.py
  • cortex/sandbox/__init__.py
  • cortex/cli.py
🧬 Code graph analysis (1)
cortex/sandbox/__init__.py (2)
cortex/sandbox/docker_sandbox.py (10)
  • DockerNotFoundError (107-110)
  • DockerSandbox (125-873)
  • SandboxAlreadyExistsError (119-122)
  • SandboxExecutionResult (95-104)
  • SandboxInfo (60-91)
  • SandboxNotFoundError (113-116)
  • SandboxState (32-38)
  • SandboxTestResult (50-56)
  • SandboxTestStatus (41-46)
  • docker_available (877-879)
cortex/sandbox/sandbox_executor.py (3)
  • CommandBlocked (43-46)
  • ExecutionResult (49-96)
  • SandboxExecutor (99-662)
🪛 markdownlint-cli2 (0.18.1)
docs/SANDBOX.md

158-158: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


169-169: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


215-215: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


249-249: Bare URL used

(MD034, no-bare-urls)

⏰ 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 (19)
README.md (1)

148-148: LGTM!

The command reference entry accurately reflects the new sandbox functionality added to the CLI.

docs/COMMANDS.md (1)

17-17: LGTM!

Comprehensive documentation with clear examples demonstrating the sandbox workflow. The quick reference entry and detailed command section align well with the CLI implementation.

Also applies to: 265-319

docs/SANDBOX.md (1)

1-59: Well-structured sandbox documentation.

The documentation provides a clear overview of the sandbox feature, practical workflow examples, and honest limitations. The troubleshooting section will help users resolve common issues.

Also applies to: 192-296

tests/test_docker_sandbox.py (4)

36-86: Good Docker detection test coverage.

Tests properly cover the three scenarios: Docker not installed, Docker installed but daemon not running, and Docker fully available. The exception handling tests verify correct error messages.


263-285: Verify mock sequence for test_test_all_pass.

The test mocks four subprocess.run calls in sequence. Ensure the order matches what the implementation actually calls:

  1. docker info (require_docker)
  2. which nginx (binary check)
  3. nginx --version (version check)
  4. dpkg --audit (conflict check)

The mock setup appears correct based on the implementation flow.


340-377: Promotion tests cover key scenarios.

The tests correctly verify:

  • Dry-run mode returns without executing (no subprocess call)
  • Attempting to promote a package not in the sandbox fails
  • Successful promotion calls the correct host apt-get install command

1-34: Comprehensive test suite with good structure.

The test file covers the main DockerSandbox functionality with appropriate mocking. The data class tests (SandboxInfo to_dict/from_dict) and convenience function tests round out the coverage.

Also applies to: 557-612

cortex/sandbox/__init__.py (1)

1-40: Clean package interface with organized exports.

The module properly exposes both the Firejail-based SandboxExecutor and the new Docker-based DockerSandbox APIs. The __all__ list clearly separates the two sandboxing approaches.

cortex/cli.py (2)

1720-1768: Well-structured argparse configuration for sandbox commands.

The subparsers correctly define all sandbox actions with appropriate arguments. The --image default, confirmation flags (-y, --yes), and force options are properly configured.


1897-1898: LGTM!

The sandbox command dispatch is correctly integrated into the main CLI routing.

cortex/sandbox/docker_sandbox.py (9)

1-28: Well-documented module header with appropriate imports.

The module docstring clearly describes the purpose and features. All necessary imports are present for the implementation.


49-105: Clean dataclass definitions with serialization support.

The SandboxInfo.from_dict() method defensively handles missing packages key with a default empty list. All fields have proper type annotations.


188-262: Robust Docker detection with proper error handling.

The implementation correctly separates availability checking (check_docker) from requirement enforcement (require_docker). Caching the docker path avoids repeated lookups, and appropriate timeouts prevent hanging on unresponsive daemons.


673-690: Host promotion uses safe subprocess invocation.

The command is constructed as a list (["sudo", "apt-get", "install", "-y", package]) which prevents shell injection. The package name is validated by apt-get itself.


810-817: Blocked command check only inspects the first argument.

The filter can be bypassed with ["bash", "-c", "systemctl start nginx"]. However, since Docker container isolation is the primary security boundary (not command filtering), this provides useful guidance without false security.

Consider documenting this limitation or enhancing the check if stricter filtering is desired.


483-639: Comprehensive package testing with smart fallbacks.

The test logic handles packages where the binary name differs from the package name (falls back to dpkg -s). The version/help check tries multiple flags (--version, -v, --help) to maximize compatibility across different programs.


843-873: LGTM!

The is_sandbox_compatible classmethod provides a useful filter for the LLM command generator. The simple string split is appropriate for this hint-based filtering use case.


714-754: Best-effort cleanup behavior.

The cleanup deletes metadata regardless of whether the container removal succeeded. This is appropriate for cleanup operations where the goal is to reset state, but could leave the container running if removal fails for unexpected reasons. The --force flag helps mitigate this.


125-167: Well-designed DockerSandbox class with clear constants.

The class docstring provides a clear usage example. The SANDBOX_BLOCKED_COMMANDS set comprehensively covers system-level commands that don't work in containers.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (2)
tests/test_docker_sandbox.py (2)

11-20: Simplify imports and path setup.

Two minor improvements:

  • Line 13: The alias shutil_module is unusual and reduces readability. Use the standard shutil name.
  • Line 20: Direct sys.path manipulation works but is fragile. Consider using pytest with proper package discovery or a conftest.py instead.
🔎 Suggested simplification
-import shutil as shutil_module
+import shutil

Then update line 62:

-        shutil_module.rmtree(self.temp_dir, ignore_errors=True)
+        shutil.rmtree(self.temp_dir, ignore_errors=True)

270-280: Consider making the promote test more flexible.

Line 279 asserts the exact command format including sudo. This tightly couples the test to implementation details and assumes sudo is always used.

Consider either:

  1. Testing that the command contains the package name and uses apt-get install without asserting the exact format, or
  2. Verifying in the test's scope documentation that sudo is an intentional requirement for promotion
Alternative assertion
-        call_args = mock_run.call_args[0][0]
-        self.assertEqual(call_args, ["sudo", "apt-get", "install", "-y", "nginx"])
+        call_args = mock_run.call_args[0][0]
+        # Verify essential components without enforcing exact command structure
+        self.assertIn("apt-get", call_args)
+        self.assertIn("install", call_args)
+        self.assertIn("nginx", call_args)
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 992de3e and fc61478.

📒 Files selected for processing (1)
  • tests/test_docker_sandbox.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs

Files:

  • tests/test_docker_sandbox.py
tests/**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

Maintain >80% test coverage for pull requests

Files:

  • tests/test_docker_sandbox.py
**/*sandbox*.py

📄 CodeRabbit inference engine (AGENTS.md)

Implement Firejail sandboxing for package operations

Files:

  • tests/test_docker_sandbox.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T12:03:24.071Z
Learning: Applies to **/*sandbox*.py : Implement Firejail sandboxing for package operations
📚 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 **/*sandbox*.py : Implement Firejail sandboxing for package operations

Applied to files:

  • tests/test_docker_sandbox.py
🧬 Code graph analysis (1)
tests/test_docker_sandbox.py (1)
cortex/sandbox/docker_sandbox.py (21)
  • DockerNotFoundError (107-110)
  • DockerSandbox (125-873)
  • SandboxAlreadyExistsError (119-122)
  • SandboxInfo (60-91)
  • SandboxNotFoundError (113-116)
  • SandboxState (32-38)
  • SandboxTestStatus (41-46)
  • docker_available (877-879)
  • check_docker (188-221)
  • require_docker (223-262)
  • create (326-417)
  • get_sandbox (775-785)
  • install (419-481)
  • test (483-639)
  • promote (641-712)
  • cleanup (714-754)
  • list_sandboxes (756-773)
  • exec_command (787-841)
  • is_sandbox_compatible (844-873)
  • to_dict (70-79)
  • from_dict (82-91)
⏰ 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)
tests/test_docker_sandbox.py (1)

76-423: Excellent test coverage and structure.

The test suite demonstrates comprehensive coverage with 11 test classes systematically validating:

  • Docker availability detection and error handling
  • Complete sandbox lifecycle (create, install, test, promote, cleanup)
  • Edge cases (non-existent sandboxes, blocked commands, empty environments)
  • Data structure serialization (SandboxInfo to_dict/from_dict)
  • Public API helpers (docker_available function)

The organization with dedicated test classes, proper mocking, and isolated fixtures follows testing best practices. This should easily exceed the >80% coverage requirement.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
tests/test_docker_sandbox.py (2)

21-21: Consider using proper package installation instead of sys.path manipulation.

While sys.path.insert is common in test files, it's fragile and can lead to import issues. Consider using proper package installation (e.g., pip install -e .) or setting PYTHONPATH in your test runner configuration.


268-273: Consider verifying subprocess.run isn't called in dry-run mode.

The test verifies the dry-run message but doesn't explicitly assert that subprocess.run wasn't called. This would strengthen confidence that dry-run truly skips execution.

🔎 Suggested enhancement
 @patch("subprocess.run")
 def test_promote_dry_run(self, mock_run: Mock) -> None:
     """Test promotion in dry-run mode."""
     result = self.create_sandbox_instance().promote("test-env", "nginx", dry_run=True)
 
     self.assertTrue(result.success)
     self.assertIn("Would run", result.message)
+    mock_run.assert_not_called()
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fc61478 and 143d1c6.

📒 Files selected for processing (1)
  • tests/test_docker_sandbox.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs

Files:

  • tests/test_docker_sandbox.py
tests/**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

Maintain >80% test coverage for pull requests

Files:

  • tests/test_docker_sandbox.py
**/*sandbox*.py

📄 CodeRabbit inference engine (AGENTS.md)

Implement Firejail sandboxing for package operations

Files:

  • tests/test_docker_sandbox.py
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T12:03:24.071Z
Learning: Applies to **/*sandbox*.py : Implement Firejail sandboxing for package operations
📚 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 **/*sandbox*.py : Implement Firejail sandboxing for package operations

Applied to files:

  • tests/test_docker_sandbox.py
📚 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 **/*.py : Type hints required in Python code

Applied to files:

  • tests/test_docker_sandbox.py
🧬 Code graph analysis (1)
tests/test_docker_sandbox.py (1)
cortex/sandbox/docker_sandbox.py (16)
  • DockerNotFoundError (107-110)
  • DockerSandbox (125-873)
  • SandboxAlreadyExistsError (119-122)
  • SandboxInfo (60-91)
  • SandboxNotFoundError (113-116)
  • SandboxState (32-38)
  • SandboxTestStatus (41-46)
  • docker_available (877-879)
  • check_docker (188-221)
  • require_docker (223-262)
  • create (326-417)
  • install (419-481)
  • test (483-639)
  • is_sandbox_compatible (844-873)
  • to_dict (70-79)
  • from_dict (82-91)
⏰ 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.12)
  • GitHub Check: test (3.10)
  • GitHub Check: test (3.11)
🔇 Additional comments (13)
tests/test_docker_sandbox.py (13)

35-53: Type hints properly implemented.

The helper functions now have complete type hints, resolving the issue flagged in the previous review. Well done!


56-83: Base test class properly implemented.

All methods have type hints and docstrings. The test infrastructure provides clean setup/teardown and helper methods for creating test fixtures.


86-132: Comprehensive Docker detection tests.

The test class thoroughly covers Docker availability scenarios: not installed, installed but not running, and fully available. Error handling and user-facing messages are properly validated.


134-172: Sandbox creation tests look solid.

The tests verify successful creation, metadata persistence, duplicate detection, and custom image support. Good coverage of the create operation.


174-214: Package installation tests cover success and failure paths.

The tests verify successful installation, failure handling, and proper exception raising for non-existent sandboxes.


217-252: Sandbox testing functionality well covered.

Tests verify both scenarios: packages with passing tests and empty sandboxes with no packages to test.


275-291: Package promotion validation and execution tests are correct.

Line 280 properly validates package presence before promotion, and lines 290-291 verify the correct apt-get install command is constructed with sudo.


294-320: Cleanup tests verify metadata removal.

The tests properly verify that cleanup removes metadata files and handles the force flag.


323-338: Sandbox listing tests are clean and effective.

The tests verify both empty and populated listing scenarios with appropriate assertions.


341-371: Command execution tests including security blocking.

Tests verify both successful execution and proper rejection of blocked system-level commands like systemctl.


374-389: Compatibility checking tests align with blocked command list.

Tests properly verify that normal commands pass while system-level commands (systemctl, service, modprobe) are correctly rejected with informative reasons.


392-417: SandboxInfo serialization tests ensure proper roundtrip conversion.

Tests verify that to_dict and from_dict correctly handle state enum serialization and all field mappings.


420-434: Convenience function tests complete the API coverage.

Tests verify the docker_available() function works correctly in both scenarios.

@Anshgrover23
Copy link
Collaborator Author

Anshgrover23 commented Dec 30, 2025

@Suyashd999 @Sahilbhatane Can I get a review on this one ?
Note: CI checks passing same as main.

@Sahilbhatane
Copy link
Collaborator

Sahilbhatane commented Dec 30, 2025

@Suyashd999 @Sahilbhatane Can I get a review on this one ? Note: CI checks passing same as main.

We already have Firejail implemented, could you change test to use Firejail sandbox only instead of docker based sandbox.
we will create a new issue for converting firejail to docker sandbox for whole project later and will assign that issue to you.

also lint after changes

@Anshgrover23
Copy link
Collaborator Author

@Sahilbhatane and I discussed over discord we are moving with this approach of docker one only.

@Suyashd999
Copy link
Collaborator

@Sahilbhatane and I discussed over discord we are moving with this approach of docker one only.

Sounds good! @sujay-d07 cam you review this please? Handing over to @sujay-d07

@Suyashd999
Copy link
Collaborator

@Anshgrover23 can you add a video of this please? Mike has instructed to have videos in PRs henceforth

@Anshgrover23
Copy link
Collaborator Author

Anshgrover23 commented Dec 31, 2025

@Suyashd999 @sujay-d07 @Sahilbhatane

Demo Video

Untitled design

@Suyashd999
Copy link
Collaborator

@Anshgrover23 thanks for the video @sujay-d07 is reviewing this and we'll merge this today once approved!

@sonarqubecloud
Copy link

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
.github/scripts/cla_check.py (1)

137-137: Consider full pagination for edge cases.

Adding per_page=100 is good, but PRs with over 100 commits won't have all authors checked. For a CLA check, missing authors could allow unsigned contributors through. Consider following the Link header for pagination or documenting this as a known limitation.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4c8a9f7 and 383a338.

📒 Files selected for processing (1)
  • .github/scripts/cla_check.py
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T12:03:24.071Z
Learning: Applies to **/*sandbox*.py : Implement Firejail sandboxing for package operations
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: Build Package
  • GitHub Check: test (3.11)
  • GitHub Check: test (3.10)
  • GitHub Check: test (3.12)
🔇 Additional comments (2)
.github/scripts/cla_check.py (2)

340-348: LGTM on bot patterns formatting.

Trailing commas on list items is a good practice for cleaner diffs when adding new patterns.


89-89: LGTM on the type hint modernization.

The str | None union syntax is appropriate for this project, which requires Python 3.10+ (confirmed in both pyproject.toml and setup.py).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Package Sandbox Testing Environment

3 participants