Draft
Conversation
Carry forward only the approved local-testing code from the old feat/sandbox branch for sandbox E2E validation. Preserved paths: - scripts/localtest/sandbox-e2e.sh - scripts/localtest/sandbox-k3d-e2e.sh - scripts/localtest/k3d-manifests/api-deploy.yaml - scripts/localtest/k3d-manifests/operator-deploy.yaml Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Rust SDK (crates/basilica-sdk/src/sandbox.rs): - CreateSandboxRequest, Sandbox, SandboxSummary, SandboxDetail types - Sandbox handle with data_plane_url(), ws_url(), exec_url() methods - Data-plane traffic goes to sandbox domains, not API server - 3 passing unit tests Python SDK (crates/basilica-sdk-python/python/basilica/sandbox.py): - SandboxClient with create(), list(), get(), delete() methods - Sandbox dataclass with data_plane_url, ws_url, exec_url properties - Same control-plane/data-plane split as Rust SDK Both SDKs connect to sandbox domains directly for data-plane ops, matching the architecture in sandboxes.md. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Adds create_sandbox, list_sandboxes, get_sandbox, delete_sandbox methods to BasilicaClient in the Rust SDK. Re-exports sandbox types from lib.rs. Registers SandboxClient and sandbox types in Python SDK __init__.py so they are accessible via `from basilica import SandboxClient`. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis pull request introduces a comprehensive sandbox management SDK layer across Python and Rust client libraries, enabling control-plane operations (create, list, get, delete). It also adds Kubernetes manifests for local development deployments and end-to-end test scripts for validating sandbox functionality across API and Kubernetes controller interfaces. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
scripts/localtest/k3d-manifests/operator-deploy.yaml (1)
86-120: Consider adding a security context for defense-in-depth.While the manifest header notes this is for local testing only, adding a basic
securityContextimproves security posture and serves as a template for production. The static analysis flags (CKV_K8S_20, CKV_K8S_23) are valid concerns.🔒 Optional: Add securityContext to the container spec
containers: - name: operator image: k3d-basilica-registry:5050/basilica-operator:latest imagePullPolicy: Always + securityContext: + allowPrivilegeEscalation: false + runAsNonRoot: true + runAsUser: 1000 + readOnlyRootFilesystem: true + capabilities: + drop: + - ALL args:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/localtest/k3d-manifests/operator-deploy.yaml` around lines 86 - 120, Add a basic securityContext to the operator container and/or Pod to address CKV_K8S_20/23: in the container spec for the container named "operator" (and optionally the Pod spec where serviceAccountName is set) add securityContext with runAsNonRoot: true, runAsUser: 1000, allowPrivilegeEscalation: false, readOnlyRootFilesystem: true and capabilities.drop: ["ALL"]; you can also add a Pod-level securityContext to enforce fsGroup or supplementalGroups if needed for volumes.crates/basilica-sdk/src/client.rs (1)
1267-1274: Consider adding unit tests for sandbox methods.The existing client methods have comprehensive unit tests (see
test_create_deployment,test_get_deployment, etc.). Adding similar tests for sandbox operations would maintain test coverage consistency.Would you like me to generate unit test stubs for the sandbox methods following the existing patterns?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/basilica-sdk/src/client.rs` around lines 1267 - 1274, Add unit test stubs for the sandbox client methods mirroring existing deployment tests: create tests for create_sandbox (and any other sandbox methods like get_sandbox, list_sandboxes) that mock the HTTP response used by Client::post/Client::get, assert the correct path "/sandboxes" and request payload mapping for create_sandbox, and verify the returned type converts to crate::sandbox::Sandbox as expected; follow the patterns in test_create_deployment and test_get_deployment to set up a fake HTTP response, call client.create_sandbox(request).await, and assert on the response fields and that the client invoked the right endpoint.crates/basilica-sdk-python/python/basilica/sandbox.py (1)
142-169: Consider documenting TTL best practice in thecreate()docstring.Per coding guidelines: "Set
--ttl/ttl_secondsfor deployments unless the user explicitly wants persistence; call out whether the resource will persist."The
ttl_secondsparameter is optional, but the docstring should clarify that sandboxes will persist until manually deleted if not set.📝 Enhance docstring with TTL guidance
def create( self, image: str, *, cpu: Optional[str] = None, memory: Optional[str] = None, env: Optional[List[SandboxEnvVar]] = None, ttl_seconds: Optional[int] = None, ) -> Sandbox: """Create a new sandbox. Args: image: Container image (must be in the server's allowlist). cpu: CPU resources (default: "1"). memory: Memory resources (default: "2Gi"). env: Environment variables to set. - ttl_seconds: Optional TTL in seconds. + ttl_seconds: TTL in seconds. If not set, the sandbox will persist + until manually deleted. Recommended: set a TTL for ephemeral + workloads (e.g., 3600 for 1 hour). Returns: A Sandbox handle with the domain for direct data-plane access. """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/basilica-sdk-python/python/basilica/sandbox.py` around lines 142 - 169, Update the create() method docstring to explicitly state the TTL behavior: explain that the ttl_seconds parameter sets a time-to-live for the created sandbox and that if ttl_seconds is not provided the sandbox will persist until manually deleted; add a brief best-practice note recommending users set ttl_seconds (or --ttl) for non-persistent/test sandboxes to avoid orphaned resources. Reference the create() method and the ttl_seconds parameter in the docstring so callers understand default persistence and recommended usage.scripts/localtest/k3d-manifests/api-deploy.yaml (1)
155-157: Usespec.ingressClassNameinstead of the deprecated annotation.The
kubernetes.io/ingress.classannotation is deprecated in favor ofspec.ingressClassName. While this still works in K3s/Traefik, using the modern field is recommended.♻️ Use ingressClassName field
metadata: name: basilica-api namespace: default - annotations: - # K3s uses Traefik by default - kubernetes.io/ingress.class: traefik spec: + ingressClassName: traefik rules:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/localtest/k3d-manifests/api-deploy.yaml` around lines 155 - 157, Replace the deprecated metadata annotation "kubernetes.io/ingress.class: traefik" with the modern Ingress field by removing that annotation and adding spec.ingressClassName: traefik under the Ingress resource's spec; locate the Ingress manifest block (the annotations section shown) and move the value into spec.ingressClassName so the Ingress uses the Traefik class without the deprecated annotation.crates/basilica-sdk-python/python/basilica/__init__.py (1)
105-112: Sandbox types imported but not exported in__all__.The sandbox types are imported at the package level but not added to
__all__. This means:
- Direct imports like
from basilica import SandboxClientwill work- Wildcard imports like
from basilica import *won't include these typesIf these types should be publicly accessible (consistent with the top-level import), add them to
__all__:♻️ Add sandbox types to __all__
# Public metadata types "EnrollMetadataResponse", "PublicDeploymentMetadataResponse", + # Sandbox types + "SandboxClient", + "Sandbox", + "SandboxDetail", + "SandboxSummary", + "SandboxEnvVar", + "CreateSandboxRequest", ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/basilica-sdk-python/python/basilica/__init__.py` around lines 105 - 112, The imported sandbox types (CreateSandboxRequest, Sandbox, SandboxClient, SandboxDetail, SandboxEnvVar, SandboxSummary) are missing from the package's __all__ export list; update the __all__ sequence in basilica.__init__ to include these symbols so wildcard imports (from basilica import *) export them, ensuring the names above are appended to the existing __all__ tuple/list.scripts/localtest/sandbox-e2e.sh (1)
91-91: Consider separating declaration and assignment to avoid masking errors.Shellcheck flags multiple instances where
local var=$(...)masks the return value of the command substitution. Withset -e, a failing command inside$(...)won't cause the script to exit.This is a low-priority improvement for a test script, but the pattern can be fixed for robustness.
♻️ Example fix for line 91
- local health=$(api_call GET "/health" 2>/dev/null || echo "{}") + local health + health=$(api_call GET "/health" 2>/dev/null || echo "{}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/localtest/sandbox-e2e.sh` at line 91, The line that declares and assigns in one step ("local health=$(api_call GET \"/health\" ...)") masks the command substitution return value; change it to declare first and assign second: call out the symbol health and the api_call GET "/health" invocation, i.e., use a separate declaration (local health) on one line and then assign health=$(api_call GET "/health" 2>/dev/null) on the next line so failures from the api_call are not masked.crates/basilica-sdk/src/sandbox.rs (2)
94-109: Harden URL builders against unexpecteddomainformatting.If
domainever arrives with a scheme (https://...) or trailing slash, current helpers generate malformed URLs. Normalize once before composing URLs.💡 Proposed refactor
impl Sandbox { + fn normalized_domain(&self) -> &str { + self.domain + .trim_start_matches("https://") + .trim_start_matches("http://") + .trim_end_matches('/') + } + /// Get the base URL for data-plane operations on this sandbox. /// Data-plane traffic goes directly to the sandbox domain, NOT through the API. pub fn data_plane_url(&self) -> String { - format!("https://{}", self.domain) + format!("https://{}", self.normalized_domain()) } /// Get the WebSocket URL for terminal access. pub fn ws_url(&self) -> String { - format!("wss://{}/ws", self.domain) + format!("wss://{}/ws", self.normalized_domain()) } /// Get the exec endpoint URL. pub fn exec_url(&self) -> String { - format!("https://{}/exec", self.domain) + format!("https://{}/exec", self.normalized_domain()) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/basilica-sdk/src/sandbox.rs` around lines 94 - 109, The URL builders (data_plane_url, ws_url, exec_url) assume self.domain has no scheme or trailing slash and can produce malformed URLs; add a small normalizing helper (e.g., Sandbox::normalized_domain or a private fn normalize_domain(&self) -> String) that trims whitespace, strips any leading "http://" or "https://" (and optional "://") and removes trailing slashes, then use that normalized string when composing the URLs in data_plane_url, ws_url, and exec_url so all outputs are correctly formed regardless of input formatting.
148-161: Prefer structured JSON assertions over substring checks.
contains(...)can pass with incorrect field names or accidental matches. Parse and assert exact keys/values instead.💡 Proposed refactor
#[test] fn test_create_request_serialization() { let req = CreateSandboxRequest { image: "registry.basilica.ai/sandbox/python:3.11".to_string(), cpu: None, memory: None, env: vec![], ttl_seconds: Some(3600), }; - let json = serde_json::to_string(&req).unwrap(); - assert!(json.contains("python:3.11")); - assert!(json.contains("3600")); - // env should be omitted when empty - assert!(!json.contains("env")); + let json = serde_json::to_value(&req).unwrap(); + assert_eq!(json["image"], "registry.basilica.ai/sandbox/python:3.11"); + assert_eq!(json["ttlSeconds"], 3600); + // env should be omitted when empty + assert!(json.get("env").is_none()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/basilica-sdk/src/sandbox.rs` around lines 148 - 161, Replace fragile substring checks in test_create_request_serialization with structured JSON assertions: serialize CreateSandboxRequest with serde_json::to_string, parse the resulting string into a serde_json::Value (or Map) and assert the exact fields and values (image == "registry.basilica.ai/sandbox/python:3.11", ttl_seconds == 3600, cpu and memory are null or absent as expected), and assert that the "env" key is not present in the top-level object; update the test to use serde_json::from_str/Value lookups instead of assert!(json.contains(...)) so field names and values are verified precisely.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/basilica-sdk-python/python/basilica/sandbox.py`:
- Around line 170-171: The SandboxClient is calling non-existent methods
_post/_get/_delete on _BasilicaClient causing AttributeError; fix by either (A)
adding Python bindings for these helpers on the Rust side (implement #[pymethod]
fn _post(&self, path: &str, json: PyObject) -> PyResult<...>, and similarly
_get/_delete, wiring them to the internal HTTP client) or (B) refactor
SandboxClient (in sandbox.py) to call the existing concrete BasilicaClient
methods instead of generic HTTP helpers — replace
self._client._post(...)/_get(...)/_delete(...) calls with the appropriate
exposed methods (e.g., create_* / get_* / delete_* methods the Rust bindings
already expose) and adapt response handling (e.g., adjust Sandbox.from_response
usage to accept whatever those methods return). Ensure consistency between the
Python stubs (.pyi) and the actual methods you add or call.
In `@scripts/localtest/k3d-manifests/api-deploy.yaml`:
- Around line 25-29: The exec_agent_image setting in config.toml currently
points to "localhost:5050/basilica-exec-agent:latest" which won't resolve from
inside pods; update the exec_agent_image value to use the in-cluster registry
hostname pattern used in sandbox-k3d-e2e.sh (e.g.,
k3d-${REGISTRY_NAME}:${REGISTRY_PORT}/basilica-exec-agent:latest) so that
exec_agent_image resolves inside the cluster; change the assignment of
exec_agent_image to reference the same registry host/port variables used by the
deployment scripts (matching the REGISTRY_NAME and REGISTRY_PORT convention).
In `@scripts/localtest/sandbox-k3d-e2e.sh`:
- Around line 253-266: The inline Dockerfile uses an invalid base image tag
"rust:1.88-slim" in the builder stage; update the FROM line in the builder stage
(the line starting with FROM rust:1.88-slim) to a valid Rust image tag
consistent with the rest of the project (e.g., the working tag used elsewhere or
"rust:1.88" or a currently published rust:<version>-slim tag); ensure the change
applies to the builder stage that builds the "basilica-api" binary for the
"$API_IMAGE" build pipeline.
---
Nitpick comments:
In `@crates/basilica-sdk-python/python/basilica/__init__.py`:
- Around line 105-112: The imported sandbox types (CreateSandboxRequest,
Sandbox, SandboxClient, SandboxDetail, SandboxEnvVar, SandboxSummary) are
missing from the package's __all__ export list; update the __all__ sequence in
basilica.__init__ to include these symbols so wildcard imports (from basilica
import *) export them, ensuring the names above are appended to the existing
__all__ tuple/list.
In `@crates/basilica-sdk-python/python/basilica/sandbox.py`:
- Around line 142-169: Update the create() method docstring to explicitly state
the TTL behavior: explain that the ttl_seconds parameter sets a time-to-live for
the created sandbox and that if ttl_seconds is not provided the sandbox will
persist until manually deleted; add a brief best-practice note recommending
users set ttl_seconds (or --ttl) for non-persistent/test sandboxes to avoid
orphaned resources. Reference the create() method and the ttl_seconds parameter
in the docstring so callers understand default persistence and recommended
usage.
In `@crates/basilica-sdk/src/client.rs`:
- Around line 1267-1274: Add unit test stubs for the sandbox client methods
mirroring existing deployment tests: create tests for create_sandbox (and any
other sandbox methods like get_sandbox, list_sandboxes) that mock the HTTP
response used by Client::post/Client::get, assert the correct path "/sandboxes"
and request payload mapping for create_sandbox, and verify the returned type
converts to crate::sandbox::Sandbox as expected; follow the patterns in
test_create_deployment and test_get_deployment to set up a fake HTTP response,
call client.create_sandbox(request).await, and assert on the response fields and
that the client invoked the right endpoint.
In `@crates/basilica-sdk/src/sandbox.rs`:
- Around line 94-109: The URL builders (data_plane_url, ws_url, exec_url) assume
self.domain has no scheme or trailing slash and can produce malformed URLs; add
a small normalizing helper (e.g., Sandbox::normalized_domain or a private fn
normalize_domain(&self) -> String) that trims whitespace, strips any leading
"http://" or "https://" (and optional "://") and removes trailing slashes, then
use that normalized string when composing the URLs in data_plane_url, ws_url,
and exec_url so all outputs are correctly formed regardless of input formatting.
- Around line 148-161: Replace fragile substring checks in
test_create_request_serialization with structured JSON assertions: serialize
CreateSandboxRequest with serde_json::to_string, parse the resulting string into
a serde_json::Value (or Map) and assert the exact fields and values (image ==
"registry.basilica.ai/sandbox/python:3.11", ttl_seconds == 3600, cpu and memory
are null or absent as expected), and assert that the "env" key is not present in
the top-level object; update the test to use serde_json::from_str/Value lookups
instead of assert!(json.contains(...)) so field names and values are verified
precisely.
In `@scripts/localtest/k3d-manifests/api-deploy.yaml`:
- Around line 155-157: Replace the deprecated metadata annotation
"kubernetes.io/ingress.class: traefik" with the modern Ingress field by removing
that annotation and adding spec.ingressClassName: traefik under the Ingress
resource's spec; locate the Ingress manifest block (the annotations section
shown) and move the value into spec.ingressClassName so the Ingress uses the
Traefik class without the deprecated annotation.
In `@scripts/localtest/k3d-manifests/operator-deploy.yaml`:
- Around line 86-120: Add a basic securityContext to the operator container
and/or Pod to address CKV_K8S_20/23: in the container spec for the container
named "operator" (and optionally the Pod spec where serviceAccountName is set)
add securityContext with runAsNonRoot: true, runAsUser: 1000,
allowPrivilegeEscalation: false, readOnlyRootFilesystem: true and
capabilities.drop: ["ALL"]; you can also add a Pod-level securityContext to
enforce fsGroup or supplementalGroups if needed for volumes.
In `@scripts/localtest/sandbox-e2e.sh`:
- Line 91: The line that declares and assigns in one step ("local
health=$(api_call GET \"/health\" ...)") masks the command substitution return
value; change it to declare first and assign second: call out the symbol health
and the api_call GET "/health" invocation, i.e., use a separate declaration
(local health) on one line and then assign health=$(api_call GET "/health"
2>/dev/null) on the next line so failures from the api_call are not masked.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 274bb0e6-a0a9-4c19-8e43-057c55048dad
📒 Files selected for processing (9)
crates/basilica-sdk-python/python/basilica/__init__.pycrates/basilica-sdk-python/python/basilica/sandbox.pycrates/basilica-sdk/src/client.rscrates/basilica-sdk/src/lib.rscrates/basilica-sdk/src/sandbox.rsscripts/localtest/k3d-manifests/api-deploy.yamlscripts/localtest/k3d-manifests/operator-deploy.yamlscripts/localtest/sandbox-e2e.shscripts/localtest/sandbox-k3d-e2e.sh
Comment on lines
+170
to
+171
| response = self._client._post("/sandboxes", json=request.to_dict()) | ||
| return Sandbox.from_response(response) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for _post, _get, _delete method definitions in the Python bindings
rg -n "_post|_get|_delete" --type rust -g '**/basilica-sdk-python/**'Repository: one-covenant/basilica
Length of output: 2813
🏁 Script executed:
# First, read the sandbox.py file to understand imports and _client initialization
head -50 crates/basilica-sdk-python/python/basilica/sandbox.pyRepository: one-covenant/basilica
Length of output: 1460
🏁 Script executed:
# Search for _BasilicaClient definition and imports
rg -n "_BasilicaClient|from basilica" crates/basilica-sdk-python/python/basilica/sandbox.py -A 3 -B 3Repository: one-covenant/basilica
Length of output: 47
🏁 Script executed:
# Check if there are .pyi stub files or Python definitions
fd -e pyi -e py crates/basilica-sdk-python/ | head -20Repository: one-covenant/basilica
Length of output: 478
🏁 Script executed:
# Look for the actual Rust module where _post, _get, _delete might be defined
rg -n "def _post|def _get|def _delete|fn _post|fn _get|fn _delete" crates/basilica-sdk-python/ -t py -t rsRepository: one-covenant/basilica
Length of output: 91
🏁 Script executed:
# Read more of sandbox.py to find imports and _client usage
cat -n crates/basilica-sdk-python/python/basilica/sandbox.py | head -100Repository: one-covenant/basilica
Length of output: 3524
🏁 Script executed:
# Search for _BasilicaClient in the entire codebase
rg -n "_BasilicaClient" crates/basilica-sdk-python/Repository: one-covenant/basilica
Length of output: 278
🏁 Script executed:
# Look for where _client is assigned in SandboxClient
rg -n "self._client" crates/basilica-sdk-python/python/basilica/sandbox.py -B 5 -A 2Repository: one-covenant/basilica
Length of output: 1277
🏁 Script executed:
# Check for .pyi stub files
fd '\\.pyi$' crates/basilica-sdk-python/Repository: one-covenant/basilica
Length of output: 47
🏁 Script executed:
# Check the __init__.py to see where _BasilicaClient comes from
cat -n crates/basilica-sdk-python/python/basilica/__init__.py | head -60Repository: one-covenant/basilica
Length of output: 2061
🏁 Script executed:
# Search for where BasilicaClient is imported
rg -n "from basilica._basilica|BasilicaClient" crates/basilica-sdk-python/python/basilica/__init__.py -B 2 -A 2Repository: one-covenant/basilica
Length of output: 3041
🏁 Script executed:
# Check if there's a .pyi stub file for the Rust module
fd . crates/basilica-sdk-python/ --type f | grep -E '\.(pyi|so|pyd)$'Repository: one-covenant/basilica
Length of output: 121
🏁 Script executed:
# Look for method definitions on BasilicaClient in the Rust module or stubs
rg -n "class BasilicaClient|def _post|def _get|def _delete" crates/basilica-sdk-python/Repository: one-covenant/basilica
Length of output: 290
🏁 Script executed:
# Check the .pyi stub file for _BasilicaClient method definitions
cat -n crates/basilica-sdk-python/python/basilica/_basilica.pyi | head -150Repository: one-covenant/basilica
Length of output: 5287
🏁 Script executed:
# Search specifically for _post, _get, _delete in the .pyi file
rg -n "_post|_get|_delete" crates/basilica-sdk-python/python/basilica/_basilica.pyiRepository: one-covenant/basilica
Length of output: 47
🏁 Script executed:
# Search for _post, _get, _delete definitions in Rust code
find crates/basilica-sdk-python -name "*.rs" -type f | xargs rg -l "_post|_get|_delete|fn post|fn get|fn delete" 2>/dev/null | head -10Repository: one-covenant/basilica
Length of output: 102
🏁 Script executed:
# Look at the Rust module structure
fd . crates/basilica-sdk-python -name "*.rs" -type f | head -20Repository: one-covenant/basilica
Length of output: 295
🏁 Script executed:
# Search in the main Python client to see if _post, _get, _delete are used elsewhere
rg -n "\._post|\._get|\._delete" crates/basilica-sdk-python/python/basilica/__init__.py -B 2 -A 2 | head -50Repository: one-covenant/basilica
Length of output: 47
🏁 Script executed:
# Look at the Rust lib.rs file
cat crates/basilica-sdk-python/src/lib.rs | head -200Repository: one-covenant/basilica
Length of output: 6511
🏁 Script executed:
# Search for _post, _get, _delete in the Python __init__.py using a simpler approach
grep -n "_post\|_get\|_delete" crates/basilica-sdk-python/python/basilica/__init__.py | head -30Repository: one-covenant/basilica
Length of output: 47
🏁 Script executed:
# Check if BasilicaClient in __init__.py has these methods
rg "def _post|def _get|def _delete" crates/basilica-sdk-python/python/basilica/Repository: one-covenant/basilica
Length of output: 47
🏁 Script executed:
# Search for _post, _get, _delete method definitions in Rust
rg "_post|_get|_delete" crates/basilica-sdk-python/src/lib.rsRepository: one-covenant/basilica
Length of output: 47
🏁 Script executed:
# Look for all pymethods in BasilicaClient Rust code
rg "fn (post|get|delete|_post|_get|_delete)" crates/basilica-sdk-python/src/lib.rs -A 3Repository: one-covenant/basilica
Length of output: 1433
🏁 Script executed:
# Check if there are any other files in the Rust module
ls -la crates/basilica-sdk-python/src/Repository: one-covenant/basilica
Length of output: 348
🏁 Script executed:
# Search the entire Rust directory for these methods
find crates/basilica-sdk-python/src -name "*.rs" -exec rg "_post|_get|_delete" {} +Repository: one-covenant/basilica
Length of output: 47
🏁 Script executed:
# Search for all methods in BasilicaClient Rust implementation
grep -n "fn " crates/basilica-sdk-python/src/lib.rs | head -50Repository: one-covenant/basilica
Length of output: 2103
🏁 Script executed:
# Look at the complete BasilicaClient in sandbox.py to understand the actual usage
cat -n crates/basilica-sdk-python/python/basilica/sandbox.py | sed -n '130,200p'Repository: one-covenant/basilica
Length of output: 2456
🏁 Script executed:
# Check if there are any POST, GET, DELETE methods (uppercase) in the Rust code
rg "fn (post|get|delete)\(" crates/basilica-sdk-python/src/lib.rs -A 2Repository: one-covenant/basilica
Length of output: 47
🏁 Script executed:
# Search for where _post is called in the entire SDK
rg "\._post\(|\._get\(|\._delete\(" crates/basilica-sdk-python/python/basilica/Repository: one-covenant/basilica
Length of output: 529
🏁 Script executed:
# Check if the Python BasilicaClient in __init__.py has _post, _get, _delete methods
grep -n "def _post\|def _get\|def _delete" crates/basilica-sdk-python/python/basilica/__init__.pyRepository: one-covenant/basilica
Length of output: 47
🏁 Script executed:
# Look at the BasilicaClient class definition in __init__.py to see its structure
sed -n '309,450p' crates/basilica-sdk-python/python/basilica/__init__.pyRepository: one-covenant/basilica
Length of output: 4843
🏁 Script executed:
# Check if these methods are supposed to be forwarded from _BasilicaClient
grep -A 20 "self._client = _BasilicaClient" crates/basilica-sdk-python/python/basilica/__init__.pyRepository: one-covenant/basilica
Length of output: 663
🏁 Script executed:
# Check if _BasilicaClient in the stub file has any methods beyond what we saw
cat crates/basilica-sdk-python/python/basilica/_basilica.pyi | grep -n "def " | head -40Repository: one-covenant/basilica
Length of output: 2225
🏁 Script executed:
# Look for __getattr__ or other dynamic method definitions
rg "__getattr__|__getattribute__" crates/basilica-sdk-python/python/basilica/Repository: one-covenant/basilica
Length of output: 47
🏁 Script executed:
# Check if SandboxClient is instantiated with a different client type
rg "SandboxClient\(" crates/basilica-sdk-python/ -A 3 -B 3Repository: one-covenant/basilica
Length of output: 47
🏁 Script executed:
# Look at tests to see how SandboxClient is actually used
find crates/basilica-sdk-python -name "*test*sandbox*" -type fRepository: one-covenant/basilica
Length of output: 47
🏁 Script executed:
# Check what methods are actually exposed by _BasilicaClient in the stub
sed -n '24,35p' crates/basilica-sdk-python/python/basilica/_basilica.pyiRepository: one-covenant/basilica
Length of output: 297
🏁 Script executed:
# Look for any mention of sandbox or sandbox-related methods
rg "sandbox|Sandbox" crates/basilica-sdk-python/src/lib.rsRepository: one-covenant/basilica
Length of output: 47
🏁 Script executed:
# Check if there are any generic _post, _get, _delete methods added as pymethods
sed -n '50,750p' crates/basilica-sdk-python/src/lib.rs | grep -n "def \|fn " | tail -50Repository: one-covenant/basilica
Length of output: 2004
🏁 Script executed:
# Search the entire lib.rs for any _post, _get, _delete definitions
wc -l crates/basilica-sdk-python/src/lib.rs
tail -100 crates/basilica-sdk-python/src/lib.rsRepository: one-covenant/basilica
Length of output: 3855
🏁 Script executed:
# Check if there are any dynamic __getattr__ implementations in Python
grep -rn "__getattr__\|__getattribute__" crates/basilica-sdk-python/python/basilica/Repository: one-covenant/basilica
Length of output: 47
🏁 Script executed:
# Verify: are _post, _get, _delete methods actually meant to exist somewhere?
rg "_post\(|_get\(|_delete\(" crates/basilica-sdk-python/ -t py --max-count 20Repository: one-covenant/basilica
Length of output: 529
🏁 Script executed:
# Final verification: look at what happens when SandboxClient.create() is called
python3 << 'EOF'
# Simulate what happens when the code tries to call _post
import sys
sys.path.insert(0, 'crates/basilica-sdk-python/python')
try:
# This will fail because _BasilicaClient has no _post method
# But let's verify the actual structure
from basilica._basilica import BasilicaClient as _BasilicaClient
client = _BasilicaClient("https://api.basilica.ai", None)
print("Available methods on _BasilicaClient:")
print([m for m in dir(client) if not m.startswith('__')])
except Exception as e:
print(f"Error: {e}")
EOFRepository: one-covenant/basilica
Length of output: 108
_BasilicaClient does not expose _post, _get, or _delete methods—implementation is missing.
The Rust bindings in lib.rs expose only specific named methods on BasilicaClient (e.g., create_deployment, get_deployment, delete_deployment), but no generic HTTP methods. The .pyi stub file also shows an empty class definition.
The SandboxClient implementation in sandbox.py calls self._client._post(), self._client._get(), and self._client._delete() (lines 170, 175, 183, 188), which do not exist. This will cause AttributeError at runtime when any SandboxClient method is invoked.
These methods must be either:
- Added to the Rust bindings as
#[pymethod]functions, or SandboxClientmust be refactored to use the existing specific methods onBasilicaClientinstead of generic HTTP operations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/basilica-sdk-python/python/basilica/sandbox.py` around lines 170 -
171, The SandboxClient is calling non-existent methods _post/_get/_delete on
_BasilicaClient causing AttributeError; fix by either (A) adding Python bindings
for these helpers on the Rust side (implement #[pymethod] fn _post(&self, path:
&str, json: PyObject) -> PyResult<...>, and similarly _get/_delete, wiring them
to the internal HTTP client) or (B) refactor SandboxClient (in sandbox.py) to
call the existing concrete BasilicaClient methods instead of generic HTTP
helpers — replace self._client._post(...)/_get(...)/_delete(...) calls with the
appropriate exposed methods (e.g., create_* / get_* / delete_* methods the Rust
bindings already expose) and adapt response handling (e.g., adjust
Sandbox.from_response usage to accept whatever those methods return). Ensure
consistency between the Python stubs (.pyi) and the actual methods you add or
call.
Comment on lines
+25
to
+29
| [sandbox] | ||
| default_timeout_seconds = 3600 | ||
| default_idle_timeout_seconds = 600 | ||
| max_sandboxes_per_user = 10 | ||
| exec_agent_image = "localhost:5050/basilica-exec-agent:latest" |
Contributor
There was a problem hiding this comment.
exec_agent_image uses localhost:5050 which won't resolve inside the cluster.
The config.toml sets exec_agent_image = "localhost:5050/basilica-exec-agent:latest", but this runs inside a container where localhost refers to the pod itself, not the host machine's registry.
Compare with sandbox-k3d-e2e.sh which correctly uses k3d-${REGISTRY_NAME}:${REGISTRY_PORT} for in-cluster image references.
🐛 Fix the registry hostname for in-cluster resolution
[sandbox]
default_timeout_seconds = 3600
default_idle_timeout_seconds = 600
max_sandboxes_per_user = 10
- exec_agent_image = "localhost:5050/basilica-exec-agent:latest"
+ exec_agent_image = "k3d-basilica-registry:5050/basilica-exec-agent:latest"📝 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.
Suggested change
| [sandbox] | |
| default_timeout_seconds = 3600 | |
| default_idle_timeout_seconds = 600 | |
| max_sandboxes_per_user = 10 | |
| exec_agent_image = "localhost:5050/basilica-exec-agent:latest" | |
| [sandbox] | |
| default_timeout_seconds = 3600 | |
| default_idle_timeout_seconds = 600 | |
| max_sandboxes_per_user = 10 | |
| exec_agent_image = "k3d-basilica-registry:5050/basilica-exec-agent:latest" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/localtest/k3d-manifests/api-deploy.yaml` around lines 25 - 29, The
exec_agent_image setting in config.toml currently points to
"localhost:5050/basilica-exec-agent:latest" which won't resolve from inside
pods; update the exec_agent_image value to use the in-cluster registry hostname
pattern used in sandbox-k3d-e2e.sh (e.g.,
k3d-${REGISTRY_NAME}:${REGISTRY_PORT}/basilica-exec-agent:latest) so that
exec_agent_image resolves inside the cluster; change the assignment of
exec_agent_image to reference the same registry host/port variables used by the
deployment scripts (matching the REGISTRY_NAME and REGISTRY_PORT convention).
Comment on lines
+253
to
+266
| cat <<'DOCKERFILE' | docker build -t "$API_IMAGE" -f - "$BACKEND_DIR" | ||
| FROM rust:1.88-slim AS builder | ||
| WORKDIR /app | ||
| RUN apt-get update && apt-get install -y pkg-config libssl-dev && rm -rf /var/lib/apt/lists/* | ||
| COPY Cargo.toml Cargo.lock ./ | ||
| COPY crates ./crates | ||
| RUN cargo build --release --package basilica-api | ||
|
|
||
| FROM debian:bookworm-slim | ||
| RUN apt-get update && apt-get install -y ca-certificates && rm -rf /var/lib/apt/lists/* | ||
| COPY --from=builder /app/target/release/basilica-api /usr/local/bin/ | ||
| EXPOSE 8080 | ||
| ENTRYPOINT ["/usr/local/bin/basilica-api"] | ||
| DOCKERFILE |
Contributor
There was a problem hiding this comment.
Same invalid Rust version in API fallback Dockerfile.
This inline Dockerfile also uses rust:1.88-slim which doesn't exist.
🐛 Fix the Rust version
-FROM rust:1.88-slim AS builder
+FROM rust:1.78-slim AS builder📝 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.
Suggested change
| cat <<'DOCKERFILE' | docker build -t "$API_IMAGE" -f - "$BACKEND_DIR" | |
| FROM rust:1.88-slim AS builder | |
| WORKDIR /app | |
| RUN apt-get update && apt-get install -y pkg-config libssl-dev && rm -rf /var/lib/apt/lists/* | |
| COPY Cargo.toml Cargo.lock ./ | |
| COPY crates ./crates | |
| RUN cargo build --release --package basilica-api | |
| FROM debian:bookworm-slim | |
| RUN apt-get update && apt-get install -y ca-certificates && rm -rf /var/lib/apt/lists/* | |
| COPY --from=builder /app/target/release/basilica-api /usr/local/bin/ | |
| EXPOSE 8080 | |
| ENTRYPOINT ["/usr/local/bin/basilica-api"] | |
| DOCKERFILE | |
| cat <<'DOCKERFILE' | docker build -t "$API_IMAGE" -f - "$BACKEND_DIR" | |
| FROM rust:1.78-slim AS builder | |
| WORKDIR /app | |
| RUN apt-get update && apt-get install -y pkg-config libssl-dev && rm -rf /var/lib/apt/lists/* | |
| COPY Cargo.toml Cargo.lock ./ | |
| COPY crates ./crates | |
| RUN cargo build --release --package basilica-api | |
| FROM debian:bookworm-slim | |
| RUN apt-get update && apt-get install -y ca-certificates && rm -rf /var/lib/apt/lists/* | |
| COPY --from=builder /app/target/release/basilica-api /usr/local/bin/ | |
| EXPOSE 8080 | |
| ENTRYPOINT ["/usr/local/bin/basilica-api"] | |
| DOCKERFILE |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/localtest/sandbox-k3d-e2e.sh` around lines 253 - 266, The inline
Dockerfile uses an invalid base image tag "rust:1.88-slim" in the builder stage;
update the FROM line in the builder stage (the line starting with FROM
rust:1.88-slim) to a valid Rust image tag consistent with the rest of the
project (e.g., the working tag used elsewhere or "rust:1.88" or a currently
published rust:<version>-slim tag); ensure the change applies to the builder
stage that builds the "basilica-api" binary for the "$API_IMAGE" build pipeline.
Closes gaps G1, G2, G3, G9, G11, G14. - sandbox-e2e.sh: use /sandboxes (not /api/v1/sandboxes), CreateSandboxInput schema (image not language, cpu/memory at top level, ttlSeconds not timeoutSeconds), data-plane requests go direct to sandbox domains via exec-agent auth (not through API proxy paths), no networkIsolation: "none" - sandbox-k3d-e2e.sh: build/deploy basilica-sandbox-operator (not generic basilica-operator), default test mode runs API + data-plane tests, inline CRD uses new schema, inline manifests use /sandboxes ingress - operator-deploy.yaml: deploy basilica-sandbox-operator with scoped RBAC - api-deploy.yaml: ingress routes /sandboxes directly to API handler, removed /api prefix path, removed unused RBAC for pods/exec and configmaps Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The Sandbox struct now has working HTTP client methods for data-plane operations, not just URL helpers: - sandbox.exec(command) - POST to https://<domain>/exec - sandbox.run(code) - POST to https://<domain>/run - sandbox.files().write(path, content) - POST to /files/write - sandbox.files().read(path) - POST to /files/read - sandbox.files().list(path) - POST to /files/list Data-plane auth uses Authorization: Bearer <exec_agent_secret> from the CreateSandboxResponse. The API now returns this secret at creation. Also updates CreateSandboxResponse to include exec_agent_secret field. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… G6) G5: SandboxClient now calls typed #[pymethods] (create_sandbox, list_sandboxes, get_sandbox, delete_sandbox) on the PyO3 BasilicaClient instead of nonexistent _post/_get/_delete methods. No more AttributeError at runtime. G6: All sandbox types added to __all__ in __init__.py: Sandbox, SandboxClient, CreateSandboxRequest, SandboxDetail, SandboxEnvVar, SandboxSummary, SandboxFiles. Also adds data-plane client methods to the Python Sandbox handle: - sandbox.exec(command) - POST to https://<domain>/exec - sandbox.run(code) - POST to https://<domain>/run - sandbox.files.write(path, content) / read(path) / list(path) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…e (G12) Add wiremock-based integration tests for all four sandbox control-plane methods: create_sandbox, list_sandboxes, get_sandbox, delete_sandbox. Also covers error paths (404, 429 rate limiting) and minimal payloads. Includes cargo fmt fixes for pre-existing formatting in sandbox module. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
K3d E2E script changes: - Build sandbox-test-api instead of full basilica-api (no Bittensor/DB) - Apply WarmPool CRD alongside Sandbox CRD (operator watches both) - Fix wait_for_deployments to fail on errors instead of || true - Set SANDBOX_IMAGE env var for E2E test runner E2E test script rewrite: - Full control-plane coverage: create, list, get, delete - Full data-plane coverage: exec, run, files/write, files/read, files/list - Snapshot test included - Negative/edge cases: invalid image, nonexistent sandbox, wrong auth - Clear pass/fail reporting with test summary (20 tests) - Uses port-forward for data-plane access Manifest fixes: - API deployment: use sandbox-test-api, add RBAC for namespaces/secrets - Operator deployment: fix health probe port (9401 not 8080), add SANDBOX_EXTRA_ALLOWED_IMAGES env var Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…luster Adds SDK integration tests that exercise all sandbox control-plane and data-plane methods against a real K3d cluster: - Rust SDK: 15 test cases in tests/sandbox_k3d.rs covering create, list, get, delete, exec, run, files (write/read/list), URL helpers, and negative cases (invalid image, nonexistent sandbox) - Python SDK: 14 test cases in tests/test_sandbox_k3d.py with same coverage - Runner script sdk-k3d-test.sh orchestrates both suites - Wired into sandbox-k3d-e2e.sh as `sdk-test` mode Also adds data_plane_base_url override to Sandbox struct (Rust + Python) to support port-forwarded local connectivity in K3d environments. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Update sandbox-k3d-e2e.sh to build and deploy the real basilica-api with dev mode config (skip_bittensor, skip_auth) and a real PostgreSQL instance. Update api-deploy.yaml with dev-mode ConfigMap, RBAC, and health checks. Adjust SDK test expectations to match real API responses. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…k_isolation - Add basilica sandbox create|list|get|delete|exec CLI subcommands - Add snapshot_create/snapshot_status to Rust + Python SDKs - Add delete/mkdir/stat to SandboxFiles in Rust + Python SDKs - Add network_isolation to CreateSandboxRequest in Rust SDK - Wire network_isolation through PyO3 binding to Python SDK - Export snapshot and file stat types from SDK crate - Remove #[allow(dead_code)] from data_plane_get Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add direct sandbox code-run support to the CLI and make local K3d sandbox routing use an explicit in-cluster override so the new cluster boundary stays safe outside dev/test. Made-with: Cursor
Ignore the local sandbox image tag file and webhook certificate directory so k3d test runs do not leave noisy generated state in the repo. Made-with: Cursor
Expose sandbox file operations, snapshot APIs, and secret rotation across the Rust SDK, Python SDK, and CLI so local and production tooling share one surface. Made-with: Cursor
Update the localtest manifests and SDK integration suites so k3d runs exercise the real sandbox API, image-tag plumbing, and webhook-aware local workflow. Made-with: Cursor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Brief description of what this PR does.
Related Issues
Closes #(issue number)
Type of Change
Changes Made
List the main changes in this PR:
Testing
How Has This Been Tested?
Describe the tests you ran to verify your changes.
cargo test)Test Configuration
Checklist
cargo fmtto format my codecargo clippyand addressed all warningsAdditional Context
Add any other context about the PR here.
Summary by CodeRabbit