Skip to content

Conversation

@robmck1995
Copy link
Contributor

@robmck1995 robmck1995 commented Jan 14, 2025

Description

  • Introduced a new ModalSandbox class in lsproxy/modal.py to encapsulate the logic for creating and managing sandboxes, including repository cloning and JWT token handling.
  • Refactored lsproxy/client.py to utilize the ModalSandbox class, simplifying the initialization process and removing direct dependencies on modal and JWT token creation.
  • Added a utility function create_named_modal_secret to handle secret creation with stable JSON stringification and SHA256 hashing.

Changes walkthrough

Relevant files
Enhancement
client.py
Refactor client to use ModalSandbox for sandbox management         

lsproxy/client.py

  • Removed direct imports and logic related to modal and JWT token
    creation.
  • Refactored to use the new ModalSandbox class for sandbox creation and
    management.
  • Simplified the initialization process by delegating responsibilities
    to ModalSandbox.
  • +5/-91   
    modal.py
    Add ModalSandbox class for enhanced sandbox management                 

    lsproxy/modal.py

  • Introduced ModalSandbox class to encapsulate sandbox creation and
    management.
  • Added create_named_modal_secret function for creating secrets with
    stable JSON and SHA256 hash.
  • Implemented logic for cloning repositories and managing JWT tokens
    within ModalSandbox.
  • +125/-0 
    💡 Usage Guide

    Checking Your Pull Request

    Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.

    Talking to CodeAnt AI

    Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:

    @codeant-ai ask: Your question here
    

    This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.

    Retrigger review

    Ask CodeAnt AI to review the PR again, by typing:

    @codeant-ai: review
    

    Check Your Repository Health

    To analyze the health of your code repository, visit our dashboard at app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.

    @codeant-ai codeant-ai bot added the size:L This PR changes 100-499 lines, ignoring generated files label Jan 14, 2025
    @codeant-ai
    Copy link

    codeant-ai bot commented Jan 14, 2025

    Pull Request Feedback 🔍

    🔒 No security issues identified
    ⚡ Recommended areas for review

    Error Handling
    The create_named_modal_secret function raises a generic Exception when the subprocess fails. It would be better to raise a more specific exception or create a custom exception class to provide more context.

    Code Duplication
    The JWT token creation logic is duplicated in both ModalSandbox and initialize_with_modal. Consider refactoring to avoid code duplication.


    # Start lsproxy
    p = sandbox.exec("lsproxy")
    sandbox = ModalSandbox(repo_url, git_token, sha, timeout, version)
    Copy link

    Choose a reason for hiding this comment

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

    Suggestion: Ensure that the ModalSandbox object is properly terminated or cleaned up in case of an exception to prevent resource leaks. [possible issue]

    Suggested change
    sandbox = ModalSandbox(repo_url, git_token, sha, timeout, version)
    try:
    sandbox = ModalSandbox(repo_url, git_token, sha, timeout, version)
    except Exception as e:
    if sandbox:
    sandbox.terminate()
    raise e

    client = cls(base_url=f"{sandbox.tunnel_url}/v1", auth_token=sandbox.jwt_token)

    print("Waiting for server start up (make take a minute)...")
    for attempt in range(180):
    Copy link

    Choose a reason for hiding this comment

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

    Suggestion: Consider adding a timeout or retry mechanism to the server startup loop to handle cases where the server fails to start within the expected time. [enhancement]

    Suggested change
    for attempt in range(180):
    for attempt in range(180):
    time.sleep(1)
    if server_is_ready():
    break
    else:
    raise TimeoutError("Server failed to start within the expected time.")

    filtered_env = {k: v for k, v in env.items() if v is not None}

    # subprocess.run already handles quoting
    keyvalues = [f"{k}={v}" for k, v in filtered_env.items()]
    Copy link

    Choose a reason for hiding this comment

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

    Suggestion: Use shlex.quote to safely construct the keyvalues list to prevent potential shell injection vulnerabilities. [security]

    Suggested change
    keyvalues = [f"{k}={v}" for k, v in filtered_env.items()]
    import shlex
    keyvalues = [f"{k}={shlex.quote(v)}" for k, v in filtered_env.items()]

    Comment on lines +41 to +45
    result = subprocess.run(
    ["modal", "secret", "create", "--force", secret_name, *keyvalues],
    capture_output=True,
    text=True,
    )
    Copy link

    Choose a reason for hiding this comment

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

    Suggestion: Add a timeout parameter to the subprocess.run call to prevent indefinite blocking in case of command execution issues. [possible issue]

    Suggested change
    result = subprocess.run(
    ["modal", "secret", "create", "--force", secret_name, *keyvalues],
    capture_output=True,
    text=True,
    )
    result = subprocess.run(
    ["modal", "secret", "create", "--force", secret_name, *keyvalues],
    capture_output=True,
    text=True,
    timeout=60,
    )

    {"GITHUB_IAT": git_token},
    f"gh-iat-token-{repo_id}"
    )
    url_parts = repo_url.split("://")
    Copy link

    Choose a reason for hiding this comment

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

    Suggestion: Check the validity of the repo_url before using it to prevent potential errors during the git clone operation. [possible bug]

    Suggested change
    url_parts = repo_url.split("://")
    if "://" not in repo_url:
    raise ValueError("Invalid repository URL")
    url_parts = repo_url.split("://")

    @robmck1995 robmck1995 merged commit 8031ea3 into main Jan 14, 2025
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Labels

    size:L This PR changes 100-499 lines, ignoring generated files

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    3 participants