Add Novita as a sandbox backend - #3846
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a Novita cloud sandbox backend with configuration, lazy registration, asynchronous execution, file operations, lifecycle controls, optional dependencies, package exports, installation hints, and unit tests. ChangesNovita sandbox backend
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant NovitaSandbox
participant AsyncSandbox
participant SandboxResult
Client->>NovitaSandbox: execute or run_command
NovitaSandbox->>NovitaSandbox: start when needed
NovitaSandbox->>AsyncSandbox: execute command with environment and timeout
AsyncSandbox-->>NovitaSandbox: return output and exit code
NovitaSandbox->>SandboxResult: create execution result
SandboxResult-->>Client: return result
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
MervinPraison
added
pipeline/blocked:ci
Greptile SummaryAdds Novita as an optional cloud sandbox backend and completes the previously missing installation guidance.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the previously reported missing Novita installation hint is now present and matches the declared optional dependency extra.
|
| Filename | Overview |
|---|---|
| src/praisonai-agents/praisonaiagents/sandbox/_sandbox_bridge.py | Adds the missing Novita extra installation hint, resolving the prior review finding. |
| src/praisonai-agents/praisonaiagents/sandbox/config.py | Adds a convenience constructor selecting the registered Novita backend. |
| src/praisonai-sandbox/praisonai_sandbox/novita.py | Implements the Novita cloud sandbox lifecycle, execution, file-management, and status interfaces. |
| src/praisonai-sandbox/praisonai_sandbox/_registry.py | Registers Novita through the existing lazy backend-loader pattern. |
| src/praisonai-sandbox/praisonai_sandbox/init.py | Exposes NovitaSandbox through the package's lazy public API. |
| src/praisonai-sandbox/pyproject.toml | Declares the Novita SDK extra and includes it in the aggregate extra. |
| src/praisonai-sandbox/tests/test_novita.py | Covers availability, startup, execution, shutdown, and status behavior. |
Reviews (2): Last reviewed commit: "fix(sandbox): add novita install hint to..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| "modal": _modal_loader, | ||
| "daytona": _daytona_loader, | ||
| "e2b": _e2b_loader, | ||
| "novita": _novita_loader, |
There was a problem hiding this comment.
Registering novita without a corresponding _EXTRA_HINTS entry means an unavailable backend recommends only pip install praisonai-sandbox, which does not install novita-sandbox and leaves the backend unavailable.
Knowledge Base Used: praisonai-sandbox
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
MervinPraison
added
pipeline/blocked:cooldown
|
Claude finished @MervinPraison's task —— View job Final Architecture Review — PR #3846 (Add Novita sandbox backend)
Verdict: ✅ ApprovePhase 1 — Architecture (AGENTS.md):
Phase 2 — Issues fixed:
Files modified by me:
Skipped: No changes to Testing:
Branch rebased and pushed to |
Registers NovitaSandbox in the same lazy-loaded registry pattern used by the existing E2B/Daytona/Modal backends, following the E2B/Daytona implementation shape (async lifecycle, NOVITA_API_KEY env var, optional novita-sandbox dependency extra).
Registering the novita backend without a corresponding _EXTRA_HINTS entry made an unavailable backend recommend only `pip install praisonai-sandbox`, which does not install novita-sandbox. Now it correctly suggests `pip install praisonai-sandbox[novita]`. Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
1b5983f to
3f4ddf0
Compare
MervinPraison
added
pipeline/blocked:stale-final
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/praisonai-sandbox/praisonai_sandbox/novita.py`:
- Around line 176-180: Update execute(), run_command(), and _run_command() to
apply SandboxConfig.env and SandboxConfig.working_dir before invoking
_sandbox.commands.run. Merge configured and call-specific environment variables
with call-specific keys taking precedence, default cwd to the configured working
directory when working_dir is omitted, and add coverage for both public entry
points.
- Around line 70-92: Update the Novita sandbox initialization around start() to
use a per-instance asyncio.Lock, rechecking _is_running after acquiring it so
concurrent calls serialize and only one AsyncSandbox.create() executes.
Initialize the lock with the instance state, preserve the existing availability
and import validation, and add a concurrent-start test asserting exactly one
AsyncSandbox.create() call.
- Around line 214-216: Update execute_file() before the open(file_path, "r")
call to normalize the caller-supplied path and validate it against an explicit
configured host-file allowlist. Reject unauthorized paths, including when no
allowlist is configured (fail closed), before reading or uploading any contents;
preserve the existing authorized-file execution flow.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0ee47acc-5940-433d-8586-2a794189ac75
📒 Files selected for processing (7)
src/praisonai-agents/praisonaiagents/sandbox/config.pysrc/praisonai-sandbox/README.mdsrc/praisonai-sandbox/praisonai_sandbox/__init__.pysrc/praisonai-sandbox/praisonai_sandbox/_registry.pysrc/praisonai-sandbox/praisonai_sandbox/novita.pysrc/praisonai-sandbox/pyproject.tomlsrc/praisonai-sandbox/tests/test_novita.py
| async def start(self) -> None: | ||
| """Start/initialize the sandbox environment.""" | ||
| if self._is_running: | ||
| return | ||
|
|
||
| if not self.is_available: | ||
| raise RuntimeError( | ||
| f"Novita is not available. Please install novita-sandbox and set NOVITA_API_KEY. {_INSTALL_HINT}" | ||
| ) | ||
|
|
||
| try: | ||
| from novita_sandbox.core import AsyncSandbox | ||
| except ImportError: | ||
| raise ImportError( | ||
| "novita-sandbox not installed. " | ||
| "Install with: pip install novita-sandbox" | ||
| ) | ||
|
|
||
| self._sandbox = await AsyncSandbox.create( | ||
| timeout=self.config.resource_limits.timeout_seconds, | ||
| ) | ||
| self._is_running = True | ||
| logger.info("Novita sandbox initialized") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate novita.py =="
fd -a 'novita\.py$' . || true
echo "== file stats outline =="
file="$(fd 'novita\.py$' . | head -n 1 || true)"
if [ -n "${file:-}" ]; then
wc -l "$file"
ast-grep outline "$file" --view expanded || true
echo "== relevant lines 1-130 =="
sed -n '1,130p' "$file" | nl -ba
fi
echo "== search for ConcurrentStart tests / locks / startup patterns =="
rg -n "concurrent|Concurrent|_running|AsyncSandbox\.create|asyncio\.Lock|test_|StartAsync|Novita.*Sandbox" -S .Repository: MervinPraison/PraisonAI
Length of output: 1193
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="$(fd 'novita\.py$' . | sed '2q;d')"
test_file="$(fd 'test_novita\.py$' . | sed '2q;d')"
echo "== novita.py 1-130 =="
sed -n '1,130p' "$file" | awk '{printf "%4d\t%s\n", NR, $0}'
echo "== tests/search relevant =="
rg -n "concurrent|Concurrent|_running|AsyncSandbox\.create|asyncio\.Lock|Mock|start\(" "$file" "$test_file" -S
echo "== stop/create usages async =="
sed -n '130,302p' "$file" | awk '{printf "%4d\t%s\n", NR+129, $0}'Repository: MervinPraison/PraisonAI
Length of output: 6137
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="$(fd 'novita\.py$' . | sed '2q;d')"
test_file="$(fd 'test_novita\.py$' . | sed '2q;d')"
echo "== novita.py full =="
python3 - <<'PY' "$file"
import sys
for i, line in enumerate(open(sys.argv[1], encoding="utf-8"), 1):
print(f"{i:4}\t{line}", end="")
PY
echo "== concurrent start probe =="
python3 - <<'PY'
import asyncio
class NovitaLike:
def __init__(self):
self._is_running = False
self._sandbox = None
self.create_count = 0
def _is_running_check(self):
return self._is_running
async def _set_running(self):
await asyncio.sleep(0.001)
self._sandbox = f"started:{id(asyncio.current_task())}"
self.create_count += 1
self._is_running = True
async def start_unprotected(self):
if self._is_running_check():
return
await self._set_running()
async def main():
for i in range(3):
sandbox = NovitaLike()
await asyncio.gather(sandbox.start_unprotected(), sandbox.start_unprotected())
print(f"attempt={i} create_count={sandbox.create_count} sandbox={sandbox._sandbox} running={sandbox._is Running_check()}")
asyncio.run(main())
PY
echo "== imports/locks in novita and tests =="
pgrep -a pgrep | head -n1 || true
python3 - <<'PY'
from pathlib import Path
for p in [Path("src/praisonai-sandbox/praisonai_sandbox/novita.py"), Path("src/praisonai-sandbox/tests/test_novita.py")]:
print(f"--- {p} ---")
text = p.read_text(encoding="utf-8")
for token in ["asyncio", "Lock", "create_task", "task_group", "concurrent", "start", "AsyncSandbox.create"]:
if token in text:
print(token)
PYRepository: MervinPraison/PraisonAI
Length of output: 4419
🏁 Script executed:
#!/bin/bash
set -euo pipefail
src="src/praisonai-sandbox/praisonai_sandbox/novita.py"
test="src/praisonai-sandbox/tests/test_novita.py"
for p in "$src" "$test"; do
echo "== full $p =="
python3 - "$p" <<'PY'
import sys
for i, line in enumerate(open(sys.argv[1], encoding="utf-8"), 1):
print(f"{i:4}\t{line}", end="")
PY
done
echo "== concurrent state race probe =="
python3 - <<'PY'
import asyncio
class NovitaLike:
def __init__(self):
self._is_running = False
self._sandbox = None
self.create_count = 0
async def _set_running(self):
await asyncio.sleep(0.001)
self._sandbox = f"started:{id(asyncio.current_task())}"
self.create_count += 1
self._is_running = True
async def start_unprotected(self):
if self._is_running:
return
await self._set_running()
async def main():
for attempt in range(1):
sandbox = NovitaLike()
await asyncio.gather(sandbox.start_unprotected(), sandbox.start_unprotected())
print(f"create_count={sandbox.create_count} sandbox={sandbox._sandbox} is_running={sandbox._is_running}")
asyncio.run(main())
PYRepository: MervinPraison/PraisonAI
Length of output: 15263
Serialize concurrent startup.
The _is_running check in start() happens before AsyncSandbox.create() awaits. Simultaneous await sandbox.start() calls can both leave the branch and create separate sandboxes; only the last self._sandbox remains killable. Protect startup with one per-instance asyncio.Lock and add a concurrent-start test that asserts exactly one AsyncSandbox.create() call.
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 83-86: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/praisonai-sandbox/praisonai_sandbox/novita.py` around lines 70 - 92,
Update the Novita sandbox initialization around start() to use a per-instance
asyncio.Lock, rechecking _is_running after acquiring it so concurrent calls
serialize and only one AsyncSandbox.create() executes. Initialize the lock with
the instance state, preserve the existing availability and import validation,
and add a concurrent-start test asserting exactly one AsyncSandbox.create()
call.
| result = await self._sandbox.commands.run( | ||
| command, | ||
| envs=env, | ||
| cwd=working_dir, | ||
| timeout=limits.timeout_seconds, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Apply SandboxConfig defaults before the SDK call.
execute() and run_command() pass None when callers omit env or working_dir. _run_command() forwards those values, so SandboxConfig.env and SandboxConfig.working_dir have no effect. Merge configured environment variables with call-specific values, with call-specific keys taking precedence. Default cwd to the configured working directory. Add coverage for both entry points.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/praisonai-sandbox/praisonai_sandbox/novita.py` around lines 176 - 180,
Update execute(), run_command(), and _run_command() to apply SandboxConfig.env
and SandboxConfig.working_dir before invoking _sandbox.commands.run. Merge
configured and call-specific environment variables with call-specific keys
taking precedence, default cwd to the configured working directory when
working_dir is omitted, and add coverage for both public entry points.
| try: | ||
| with open(file_path, "r") as f: | ||
| code = f.read() |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate file and inspect context:"
fd -a 'novita.py' . | sed 's#^\./##'
file=$(fd 'novita.py' . | head -n1)
if [ -n "${file:-}" ]; then
wc -l "$file"
ast-grep outline "$file" || true
echo "=== relevant lines 1-260 ==="
sed -n '1,260p' "$file" | nl -ba
fi
echo "=== searches for execute_file and related config/default params ==="
rg -n "def execute_file|execute_file\\(|file_path|allowlist|allowed|sandbox|SandboxConfig|working_dir|env\\)" .Repository: MervinPraison/PraisonAI
Length of output: 810
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=$(fd 'novita.py' . | head -n1)
echo "file=$file"
echo "=== relevant lines 1-280 ==="
sed -n '1,280p' "$file" | awk '{printf "%5d\t%s\n", NR, $0}'
echo "=== execute_file definition and callers/test snippets ==="
rg -n -C 6 "def execute_file|execute_file\\(|file_path|allowed|allowlist|working_dir" .Repository: MervinPraison/PraisonAI
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=$(fd 'novita.py' . | head -n1)
test_file=$(fd 'test_novita.py' . | head -n1)
echo "=== novita.py execute_file section ==="
sed -n '180,270p' "$file" | awk '{printf "%5d\t%s\n", NR+179, $0}'
echo "=== novita.py init/defaults section ==="
sed -n '28,130p' "$file" | awk '{printf "%5d\t%s\n", NR+27, $0}'
echo "=== novita.py execute_file signature/imports relevant ==="
sed -n '1,40p' "$file" | awk '{printf "%5d\t%s\n", NR, $0}'
echo "=== test_novita.py execute_file calls ==="
if [ -n "${test_file:-}" ]; then
rg -n -C 8 "execute_file\\(|def test_|file_path|working_dir" "$test_file"
fi
echo "=== scoped policy/search in praisonai-sandbox ==="
sed -n '1,280p' "$file" | awk '{printf "%5d\t%s\n", NR, $0}' | rg -n "file_path|allowed|allowlist|working_dir|file path|sandbox|execute_file"Repository: MervinPraison/PraisonAI
Length of output: 15552
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import ast
from pathlib import Path
path = Path("src/praisonai-sandbox/praisonai_sandbox/novita.py")
tree = ast.parse(path.read_text())
for cls in tree.body:
if isinstance(cls, ast.ClassDef) and cls.name == "NovitaSandbox":
for fn in cls.body:
if isinstance(fn, ast.AsyncFunctionDef) and fn.name in {"__init__", "execute_file"}:
print(f"{fn.name}:")
print(f" signature={ast.dump(ast.Module(body=[fn], type_ignores=[]), include_attributes=False)}")
opens = []
for node in ast.walk(fn):
if isinstance(node, ast.With) and any(
isinstance(t.context_expr, ast.Name) and t.context_expr.id == "open"
for t in node.items
):
opens.append(node)
print(f" open_statements={len(opens)}")
for with_node in opens:
open_call = next(
node
for t in with_node.items
if isinstance(t.context_expr, ast.Call)
and isinstance(t.context_expr.func, ast.Name)
and t.context_expr.func.id == "open"
for node in [t]
)
call = open_call.context_expr
print(f" open_file_arg={ast.dump(call.args[0]) if call.args else None}")
print(f" open_mode_arg={ast.dump(call.args[1]) if len(call.args) > 1 else None}")
print(f" open_kwargs_arg={ast.dump(call.keywords[0].value) if call.keywords and call.keywords[0].arg == 'mode' else None}")
PY
echo "=== read-only path normalization probe using Python stdlib only ==="
python3 - <<'PY'
import os
print("os.getcwd:", os.getcwd())
print("os.path.abspath('../../secret.py')", os.path.abspath(os.path.join("/app/praisonai/nov", "../../secret.py")))
print("os.path.realpath", os.path.realpath(os.path.join("/app/praisonai/nov", "../../secret.py")))
print("commonprefix example", os.path.commonprefix(["/app/work/project", os.path.abspath(os.path.join("/app/praisonai/nov", "../../secret.py"))]))
PYRepository: MervinPraison/PraisonAI
Length of output: 5354
Enforce the host file access policy.
execute_file() opens the caller-supplied file_path on the host before the sandbox starts, then uploads its contents to Novita. For untrusted caller input, normalize the path and authorize it against an explicit configured allowlist with a fail-closed default.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 214-214: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(file_path, "r")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/praisonai-sandbox/praisonai_sandbox/novita.py` around lines 214 - 216,
Update execute_file() before the open(file_path, "r") call to normalize the
caller-supplied path and validate it against an explicit configured host-file
allowlist. Reject unauthorized paths, including when no allowlist is configured
(fail closed), before reading or uploading any contents; preserve the existing
authorized-file execution flow.
2 participants
Summary
Adds
NovitaSandbox, a new cloud sandbox backend built onnovita-sandbox, following the same pattern as the existingdaytona/e2bbackends.Changes
praisonai_sandbox/novita.py—NovitaSandboximplementingSandboxProtocol(start/stop/execute/run_command/read_file/write_file/list_files/get_status/cleanup/reset), readingNOVITA_API_KEYfrom the environment.praisonai_sandbox/_registry.py— registers"novita"in_BUILTIN_SANDBOXES.praisonai_sandbox/__init__.py— exportsNovitaSandbox.praisonaiagents/sandbox/config.py— addsSandboxConfig.novita()classmethod.pyproject.toml— adds anovitaoptional-dependency extra (novita-sandbox>=2.0.0).README.md— listsnovitaalongside the other backends.tests/test_novita.py— unit tests mirroringtest_daytona.py/test_e2b_sandbox.py.Verification
pytest src/praisonai-sandbox/tests/test_novita.py -q→ 9 passed.praisonai-sandboxsuite passes with no regressions from this change.Summary by CodeRabbit
New Features
Bug Fixes