Steven passynkov/bug/refactor#5
Merged
Merged
Conversation
steven-passynkov
force-pushed
the
steven-passynkov/bug/refactor
branch
from
b0181d3 to
7e22727
Compare
Contributor
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReorganizes internals: splits models/config/constants into Changes
Sequence Diagram(s)sequenceDiagram
participant User as User Code
participant Client as DesktopClient.wait_until_ready
participant Retry as tenacity.retry
participant Stream as Desktop.status_stream (SSE)
participant Transport as HTTP Transport
User->>Client: wait_until_ready(sandbox, timeout)
Client->>Retry: enter retry loop (bounded by timeout)
Retry->>Stream: open status_stream(sandbox, deadline)
Stream->>Transport: GET /v1/sandbox/{id}/desktop/status (SSE)
Transport-->>Stream: SSE lines
Stream-->>Client: parse events -> DesktopProcessStatusList
alt status == "running"
Client-->>User: return (ready)
else transient error or non-running
Client->>Retry: raise to retry loop (retry/backoff)
end
alt overall timeout exceeded
Client-->>User: raise Leap0TimeoutError
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 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 docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (9)
leap0/common/code_interpreter.py (2)
45-45: Remove duplicateLiteraltype definition.
StreamEventTypeLiteral(line 45) andStreamEventType(line 177) define the same literal type. Consider consolidating to a single definition for consistency.♻️ Proposed fix
-StreamEventTypeLiteral = Literal["stdout", "stderr", "exit", "error"] +StreamEventType = Literal["stdout", "stderr", "exit", "error"]Then remove the duplicate on line 177 and use
StreamEventTypeconsistently throughout.Also applies to: 177-177
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@leap0/common/code_interpreter.py` at line 45, There are two identical Literal type definitions: StreamEventTypeLiteral and StreamEventType; remove the duplicate by deleting StreamEventTypeLiteral and update all type annotations to use the single StreamEventType symbol (or vice versa), e.g., replace any references to StreamEventTypeLiteral with StreamEventType, remove the redundant definition, and run a quick type-check to ensure no remaining references to the removed name in functions/classes that accept/return stream event types.
109-116: Consider handling invalid base64 data gracefully.The
png_bytes(),jpeg_bytes(), andpdf_bytes()methods will raisebinascii.Errorif the base64 data is malformed. If API responses could contain invalid data, consider wrapping with try/except or documenting this behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@leap0/common/code_interpreter.py` around lines 109 - 116, The png_bytes, jpeg_bytes, and pdf_bytes methods call base64.b64decode directly and will raise binascii.Error (or ValueError) on malformed input; wrap the base64.b64decode calls in each of png_bytes, jpeg_bytes, and pdf_bytes with a try/except that catches binascii.Error/ValueError and either returns None (or re-raises a clearer custom exception) and optionally logs the decode failure so callers don't get uncaught decode errors; ensure you reference the existing method names (png_bytes, jpeg_bytes, pdf_bytes) and the base64.b64decode call when making the change.leap0/pty.py (1)
91-103: Avoid double-wrapping errors inconnect.With Line 91 adding
@intercept_errors(...), the broadexcept Exceptionblock can become redundant and may reduce error specificity.Proposed patch
from ._utils.errors import intercept_errors from ._utils.url import websocket_url_from_http -from .common.errors import Leap0WebSocketError from .common.pty import PtyConnection, PtyListResponseDict, PtySession, PtySessionInfoDict from .common.sandbox import SandboxRef, sandbox_id_of @@ def connect(self, sandbox: SandboxRef, session_id: str, **kwargs: Any) -> PtyConnection: @@ url = self.websocket_url(sandbox, session_id) - try: - websocket = connect(url, additional_headers={self._transport.auth_header: self._transport.auth_value}, **kwargs) - except Exception as exc: - raise Leap0WebSocketError(str(exc)) from exc + websocket = connect( + url, + additional_headers={self._transport.auth_header: self._transport.auth_value}, + **kwargs, + ) return PtyConnection(websocket=websocket)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@leap0/pty.py` around lines 91 - 103, The connect method is double-wrapping exceptions because it already has `@intercept_errors`("Failed to connect to PTY session: ") and also catches Exception to raise Leap0WebSocketError; remove the broad try/except around the connect(url, ...) call (or limit it to catch only specific, handled exceptions) so that intercept_errors can produce a single, informative wrapper; update the connect function body (referencing websocket_url, the connect(...) call and Leap0WebSocketError) to let exceptions propagate or be caught selectively rather than always converting every Exception into Leap0WebSocketError.examples/desktop.py (1)
18-26: Preserve the last readiness error to improve timeout diagnostics.At Line 20, all
Leap0Errorexceptions are silently ignored, which can make the final timeout hard to debug. Consider keeping the last error and appending it to theTimeoutError.Proposed patch
def wait_for_desktop(client: Leap0Client, sandbox: SandboxRef, *, timeout_seconds: float = 60.0) -> None: deadline = time.monotonic() + timeout_seconds + last_error: Leap0Error | None = None while time.monotonic() < deadline: sandbox_status = client.sandboxes.get(sandbox) if sandbox_status.state == "running": try: health = client.desktop.health(sandbox) except Leap0Error: - pass + last_error = _ else: if health.ok and health.state == "ready": return time.sleep(0.25) - raise TimeoutError(f"Sandbox {sandbox} did not become ready within {timeout_seconds:.0f}s") + suffix = f" Last error: {last_error}" if last_error is not None else "" + raise TimeoutError( + f"Sandbox {sandbox} did not become ready within {timeout_seconds:.0f}s.{suffix}" + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/desktop.py` around lines 18 - 26, The loop calling client.desktop.health(sandbox) swallows Leap0Error, losing the last failure context; introduce a variable (e.g., last_err = None) before the loop, assign last_err = e in the except Leap0Error as e block (and optionally capture non-ready health responses), and when raising the TimeoutError at the end include the preserved error details (either by appending its message to the TimeoutError string or raising the TimeoutError from last_err using "raise TimeoutError(...) from last_err") so the final timeout includes the last readiness error for diagnostics.tests/test_process.py (1)
7-12: Tighten transport-call assertion to reduce false positives.The
"/process/execute" in ...check is permissive. Consider asserting the exact method/path tuple.Proposed patch
class TestProcessClient: def test_execute(self, mock_transport): mock_transport.request_json.return_value = {"exit_code": 0, "result": "hello"} result = ProcessClient(mock_transport).execute("sbx-1", command="echo hello") assert result.exit_code == 0 assert result.result == "hello" - assert "/process/execute" in mock_transport.request_json.call_args[0][1] + mock_transport.request_json.assert_called_once() + method, path = mock_transport.request_json.call_args[0][:2] + assert method == "POST" + assert path == "/v1/sandbox/sbx-1/process/execute"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_process.py` around lines 7 - 12, The test uses a permissive substring check for the transport call; update test_execute to assert the exact path or method/path tuple on mock_transport.request_json to avoid false positives — replace the "`assert '/process/execute' in mock_transport.request_json.call_args[0][1]`" with an exact equality assertion such as checking `mock_transport.request_json.call_args[0][1] == '/process/execute'` or, if the transport sends (method, path, ...), assert `mock_transport.request_json.call_args[0][:2] == ('POST', '/process/execute')`; update the assertion in test_execute accordingly for the shape used by ProcessClient and mock_transport.request_json.tests/test_ssh.py (1)
16-19: Consider verifying the full endpoint path including sandbox ID.The assertion only checks for
/ssh/accesssubstring, but doesn't verify that the sandbox ID"sbx-1"was correctly included in the path. This could miss bugs where the sandbox ID is omitted or malformed.💡 Suggested improvement
def test_delete_access(self, mock_transport): mock_transport.request.return_value = MagicMock(status_code=204) SshClient(mock_transport).delete_access("sbx-1") - assert "/ssh/access" in mock_transport.request.call_args[0][1] + path = mock_transport.request.call_args[0][1] + assert "/ssh/access" in path + assert "sbx-1" in path🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_ssh.py` around lines 16 - 19, The test_delete_access assertion is too loose; update it to verify the full endpoint path includes the sandbox ID by asserting that the request path argument passed to mock_transport.request contains the exact segment with the sandbox id (e.g. "/ssh/access/sbx-1") or compare the full path string directly; target the call to mock_transport.request used in test_delete_access (and ensure SshClient.delete_access is called with "sbx-1") so the assertion fails if the sandbox id is missing or malformed.tests/test_filesystem.py (1)
15-19: Consider movingMagicMockimport to module level for consistency.The
MagicMockimport is at the module level intests/test_ssh.py(line 3) but inline here. Keeping imports consistent across test files improves maintainability.💡 Suggested change
from __future__ import annotations import pytest +from unittest.mock import MagicMock from leap0.filesystem import FilesystemClient, _parse_multipart_response from leap0.common.filesystem import FileEditThen remove the inline import:
def test_mkdir(self, mock_transport): - from unittest.mock import MagicMock mock_transport.request.return_value = MagicMock(status_code=204)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_filesystem.py` around lines 15 - 19, Move the inline import of MagicMock out of test_mkdir and into the module-level imports to match other test files; specifically, add "from unittest.mock import MagicMock" at the top of tests/test_filesystem.py and then remove the inline "from unittest.mock import MagicMock" line inside the test_mkdir function so the test uses the module-level MagicMock for mock_transport.request return values.tests/test_templates.py (1)
15-15: Prefix unused variable with underscore.The
kwargsvariable is unpacked but never used. Prefix it with an underscore to indicate it's intentionally ignored.♻️ Suggested fix
- args, kwargs = mock_transport.request_json.call_args + args, _kwargs = mock_transport.request_json.call_args🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_templates.py` at line 15, The unpacked but unused variable kwargs in the test assertion should be prefixed with an underscore to indicate it's intentionally ignored; update the destructuring of mock_transport.request_json.call_args so it assigns args, _kwargs = mock_transport.request_json.call_args (or args, _ = ...) wherever that line appears in tests/test_templates.py to remove the unused-variable warning.leap0/common/template.py (1)
69-77: Removetype: ignoresuppression and add defensive type checks for robust payload handling inTemplate.from_dict.Lines 72-77 use
type: ignore[arg-type]to suppress checks and.get()on required TypedDict fields. While tests confirm the API currently sends proper booleans, thebool(data.get("is_system", False))pattern will silently coerce string values like"false"toTrue, masking payload drift. Replace coercions with explicitisinstance()checks:Proposed defensive normalization
`@classmethod` def from_dict(cls, data: UploadTemplateResponseDict) -> Template: ic = data.get("image_config") + raw_is_system = data.get("is_system", False) return cls( - id=data.get("id", ""), # type: ignore[arg-type] - name=data.get("name", ""), # type: ignore[arg-type] - digest=data.get("digest", ""), # type: ignore[arg-type] + id=data.get("id", "") if isinstance(data.get("id", ""), str) else "", + name=data.get("name", "") if isinstance(data.get("name", ""), str) else "", + digest=data.get("digest", "") if isinstance(data.get("digest", ""), str) else "", image_config=ImageConfig.from_dict(ic) if isinstance(ic, dict) else None, - is_system=bool(data.get("is_system", False)), # type: ignore[arg-type] - created_at=data.get("created_at", ""), # type: ignore[arg-type] + is_system=raw_is_system if isinstance(raw_is_system, bool) else False, + created_at=data.get("created_at", "") if isinstance(data.get("created_at", ""), str) else "", )This pattern appears in other
from_dictmethods across the codebase; consider a similar refactor for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@leap0/common/template.py` around lines 69 - 77, Template.from_dict is currently using type: ignore and loose coercions that can mask payload drift; remove the type: ignore suppressions and add explicit type checks: validate id, name, digest and created_at are str (fallback to "" if not), only call ImageConfig.from_dict(ic) when ic is a dict, and set is_system to the boolean value only if isinstance(value, bool) (fallback to False otherwise); update the method body (Template.from_dict) to perform these defensive checks for each field instead of using bool(...) or .get(...) with type ignores.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@leap0/__init__.py`:
- Around line 44-45: The TypedDicts GlobResponseDict and GrepResponseDict are
imported but not exported in the module's public API; update the __all__ list in
leap0/__init__.py to include "GlobResponseDict" and "GrepResponseDict" (insert
them alphabetically among the other TypedDict/export names) so they are part of
the package's public exports.
In `@leap0/_utils/errors.py`:
- Around line 23-43: The decorator currently only catches exceptions raised when
the wrapped function is called, not those raised during iteration of a returned
generator; update the decorator (the inner function named decorator/wrapper in
leap0/_utils/errors.py) to detect when fn(*args, **kwargs) returns a
generator/iterator and, in that case, return a new generator that yields from
the original inside a try/except around each yield (or around the iteration),
applying the same Leap0Error normalization logic (message_prefix adjustments and
exc.args population) and calling _raise_wrapped for non-Leap0 exceptions; this
ensures CodeInterpreterClient.execute_stream and DesktopClient.status_stream are
wrapped for errors that occur during streaming iteration as well as during
generator creation.
In `@leap0/_utils/stream.py`:
- Around line 25-27: The SSE line-handling only strips "\r" (stripped =
line.rstrip("\r")) so lines ending with "\n" aren't treated as empty event
delimiters and multiple events can be merged; update the logic in the SSE
parsing routine (the code that sets stripped and checks if stripped == "" and
uses buffer) to remove both CR and LF (e.g., use rstrip("\r\n") or otherwise
strip "\n" too) so that raw "\n"-terminated lines correctly signal event
boundaries and flush the buffer per event.
In `@leap0/common/config.py`:
- Around line 46-49: The current assignment in the config initializer treats
only None as unset for self.base_url and self.sandbox_domain, allowing empty
strings to slip through and create bad URLs; update the logic in the initializer
where self.base_url and self.sandbox_domain are set so that blank or
whitespace-only strings are treated as unset (e.g., check truthiness or strip()
before choosing) and then fall back to os.environ.get("LEAP0_BASE_URL") or
DEFAULT_BASE_URL and os.environ.get("LEAP0_SANDBOX_DOMAIN") or
DEFAULT_SANDBOX_DOMAIN respectively; update the code paths that reference
self.base_url/self.sandbox_domain to rely on these normalized values.
In `@leap0/common/desktop.py`:
- Line 237: The list comprehension constructing DesktopProcessStatus objects
should validate each element is a mapping/dict before calling
DesktopProcessStatus.from_dict to avoid AttributeError on malformed entries;
update the expression that builds items (currently using
DesktopProcessStatus.from_dict(cast(DesktopProcessStatusDict, cast(object,
item)))) to first check isinstance(item, dict) (or collections.abc.Mapping) and
only call DesktopProcessStatus.from_dict for those, optionally skipping or
logging non-dict entries, and remove the redundant cast(...) calls; this mirrors
the defensive check used around line 253 and ensures safe handling of nested
items in the DesktopProcessList/desktop module.
- Around line 102-103: Implement a safe integer parsing helper (e.g.,
safe_int(value, default=0)) and use it wherever the code currently calls
int(data.get(...)) so deserialization tolerates None or invalid strings; replace
the int(...) calls for the fields width and height, the nested
desktop/pid/x/y/width/height group, x and y at lines ~145, size_bytes, pid at
~220, and running and total at ~238-239 with safe_int(...) (or parse
booleans/ints appropriately for running if it should be boolean), updating the
deserialization functions/classes in leap0/common/desktop.py to call this helper
instead of int(data.get(...)) so malformed or null values fall back to the
specified default rather than raising.
In `@leap0/common/pty.py`:
- Around line 57-63: The recv method in class using websocket.recv() contains an
unreachable fallback b"".join(message); remove that line and either return a
clear error or assert for unexpected types after the isinstance checks (e.g.,
raise TypeError or use assert False) so behavior is explicit; update the recv()
function that calls self.websocket.recv() to only handle str and bytes and
eliminate the dead b"".join(message) fallback.
In `@leap0/common/sandbox.py`:
- Around line 61-65: The from_dict classmethod in Sandbox (signature:
from_dict(cls, data: SandboxCreateResponseDict)) currently indexes data["id"]
directly which can raise KeyError on malformed payloads; update it to explicitly
validate that "id" exists and is a non-empty string (or expected type) before
using it, and raise a controlled parse/validation error (e.g., ValueError or a
project-specific ParseError) with a clear message if the check fails, then
proceed to construct the Sandbox instance using the validated id.
In `@leap0/desktop.py`:
- Around line 318-321: The health method currently calls _request_json("GET",
sandbox, "/api/healthz") which only treats a 200 as success and will raise on a
503; change the call in Desktop.health so the /api/healthz response with status
503 is accepted and its JSON parsed into DesktopHealth (for example by passing
an allow/expected statuses option such as 200 and 503 or using a parameter like
expected_statuses to _request_json), ensuring DesktopHealthDict is returned even
when the desktop is unhealthy; keep the method signature (health(self, sandbox:
SandboxRef) -> DesktopHealth) and preserve the `@intercept_errors` decorator.
---
Nitpick comments:
In `@examples/desktop.py`:
- Around line 18-26: The loop calling client.desktop.health(sandbox) swallows
Leap0Error, losing the last failure context; introduce a variable (e.g.,
last_err = None) before the loop, assign last_err = e in the except Leap0Error
as e block (and optionally capture non-ready health responses), and when raising
the TimeoutError at the end include the preserved error details (either by
appending its message to the TimeoutError string or raising the TimeoutError
from last_err using "raise TimeoutError(...) from last_err") so the final
timeout includes the last readiness error for diagnostics.
In `@leap0/common/code_interpreter.py`:
- Line 45: There are two identical Literal type definitions:
StreamEventTypeLiteral and StreamEventType; remove the duplicate by deleting
StreamEventTypeLiteral and update all type annotations to use the single
StreamEventType symbol (or vice versa), e.g., replace any references to
StreamEventTypeLiteral with StreamEventType, remove the redundant definition,
and run a quick type-check to ensure no remaining references to the removed name
in functions/classes that accept/return stream event types.
- Around line 109-116: The png_bytes, jpeg_bytes, and pdf_bytes methods call
base64.b64decode directly and will raise binascii.Error (or ValueError) on
malformed input; wrap the base64.b64decode calls in each of png_bytes,
jpeg_bytes, and pdf_bytes with a try/except that catches
binascii.Error/ValueError and either returns None (or re-raises a clearer custom
exception) and optionally logs the decode failure so callers don't get uncaught
decode errors; ensure you reference the existing method names (png_bytes,
jpeg_bytes, pdf_bytes) and the base64.b64decode call when making the change.
In `@leap0/common/template.py`:
- Around line 69-77: Template.from_dict is currently using type: ignore and
loose coercions that can mask payload drift; remove the type: ignore
suppressions and add explicit type checks: validate id, name, digest and
created_at are str (fallback to "" if not), only call ImageConfig.from_dict(ic)
when ic is a dict, and set is_system to the boolean value only if
isinstance(value, bool) (fallback to False otherwise); update the method body
(Template.from_dict) to perform these defensive checks for each field instead of
using bool(...) or .get(...) with type ignores.
In `@leap0/pty.py`:
- Around line 91-103: The connect method is double-wrapping exceptions because
it already has `@intercept_errors`("Failed to connect to PTY session: ") and also
catches Exception to raise Leap0WebSocketError; remove the broad try/except
around the connect(url, ...) call (or limit it to catch only specific, handled
exceptions) so that intercept_errors can produce a single, informative wrapper;
update the connect function body (referencing websocket_url, the connect(...)
call and Leap0WebSocketError) to let exceptions propagate or be caught
selectively rather than always converting every Exception into
Leap0WebSocketError.
In `@tests/test_filesystem.py`:
- Around line 15-19: Move the inline import of MagicMock out of test_mkdir and
into the module-level imports to match other test files; specifically, add "from
unittest.mock import MagicMock" at the top of tests/test_filesystem.py and then
remove the inline "from unittest.mock import MagicMock" line inside the
test_mkdir function so the test uses the module-level MagicMock for
mock_transport.request return values.
In `@tests/test_process.py`:
- Around line 7-12: The test uses a permissive substring check for the transport
call; update test_execute to assert the exact path or method/path tuple on
mock_transport.request_json to avoid false positives — replace the "`assert
'/process/execute' in mock_transport.request_json.call_args[0][1]`" with an
exact equality assertion such as checking
`mock_transport.request_json.call_args[0][1] == '/process/execute'` or, if the
transport sends (method, path, ...), assert
`mock_transport.request_json.call_args[0][:2] == ('POST', '/process/execute')`;
update the assertion in test_execute accordingly for the shape used by
ProcessClient and mock_transport.request_json.
In `@tests/test_ssh.py`:
- Around line 16-19: The test_delete_access assertion is too loose; update it to
verify the full endpoint path includes the sandbox ID by asserting that the
request path argument passed to mock_transport.request contains the exact
segment with the sandbox id (e.g. "/ssh/access/sbx-1") or compare the full path
string directly; target the call to mock_transport.request used in
test_delete_access (and ensure SshClient.delete_access is called with "sbx-1")
so the assertion fails if the sandbox id is missing or malformed.
In `@tests/test_templates.py`:
- Line 15: The unpacked but unused variable kwargs in the test assertion should
be prefixed with an underscore to indicate it's intentionally ignored; update
the destructuring of mock_transport.request_json.call_args so it assigns args,
_kwargs = mock_transport.request_json.call_args (or args, _ = ...) wherever that
line appears in tests/test_templates.py to remove the unused-variable warning.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 87c07f5f-3111-43bc-b64d-43dfa76aadb1
📒 Files selected for processing (71)
examples/desktop.pyleap0/__init__.pyleap0/_transport.pyleap0/_types.pyleap0/_utils.pyleap0/_utils/__init__.pyleap0/_utils/encoding.pyleap0/_utils/errors.pyleap0/_utils/stream.pyleap0/_utils/url.pyleap0/client.pyleap0/code_interpreter.pyleap0/common/__init__.pyleap0/common/code_interpreter.pyleap0/common/config.pyleap0/common/desktop.pyleap0/common/errors.pyleap0/common/filesystem.pyleap0/common/git.pyleap0/common/lsp.pyleap0/common/process.pyleap0/common/pty.pyleap0/common/sandbox.pyleap0/common/snapshot.pyleap0/common/ssh.pyleap0/common/template.pyleap0/config.pyleap0/constants.pyleap0/desktop.pyleap0/exceptions.pyleap0/filesystem.pyleap0/git.pyleap0/lsp.pyleap0/models.pyleap0/process.pyleap0/pty.pyleap0/sandboxes.pyleap0/snapshots.pyleap0/ssh.pyleap0/templates.pytests/__init__.pytests/_utils/__init__.pytests/_utils/test_encoding.pytests/_utils/test_stream.pytests/_utils/test_url.pytests/common/__init__.pytests/common/test_code_interpreter.pytests/common/test_config.pytests/common/test_desktop.pytests/common/test_errors.pytests/common/test_filesystem.pytests/common/test_git.pytests/common/test_lsp.pytests/common/test_process.pytests/common/test_pty.pytests/common/test_sandbox.pytests/common/test_snapshot.pytests/common/test_ssh.pytests/common/test_template.pytests/conftest.pytests/test_clients.pytests/test_filesystem.pytests/test_git.pytests/test_models.pytests/test_process.pytests/test_sandboxes.pytests/test_snapshots.pytests/test_ssh.pytests/test_templates.pytests/test_transport.pytests/test_utils.py
💤 Files with no reviewable changes (9)
- leap0/exceptions.py
- tests/test_models.py
- leap0/constants.py
- leap0/config.py
- leap0/_types.py
- tests/test_utils.py
- tests/test_clients.py
- leap0/_utils.py
- leap0/models.py
There was a problem hiding this comment.
♻️ Duplicate comments (1)
leap0/__init__.py (1)
100-202:⚠️ Potential issue | 🟡 MinorAdd missing
GlobResponseDict/GrepResponseDictto__all__.These are imported at Line [55]-Line [56] but still not exported from the public API list.
🔧 Proposed fix
"ExecutionLogsDict", "FileInfoDict", + "GlobResponseDict", + "GrepResponseDict", "GitCommitResponseDict", "GitResultDict",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@leap0/__init__.py` around lines 100 - 202, The __all__ export list is missing the exported names for GlobResponseDict and GrepResponseDict even though those types are imported elsewhere; update the __all__ list to include "GlobResponseDict" and "GrepResponseDict" (add those exact symbols into the existing list alongside the other *Dict entries) so they become part of the package public API.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@leap0/__init__.py`:
- Around line 100-202: The __all__ export list is missing the exported names for
GlobResponseDict and GrepResponseDict even though those types are imported
elsewhere; update the __all__ list to include "GlobResponseDict" and
"GrepResponseDict" (add those exact symbols into the existing list alongside the
other *Dict entries) so they become part of the package public API.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7d6f1990-b1dc-4406-8ff1-0072ed99810f
📒 Files selected for processing (13)
leap0/__init__.pyleap0/_utils/stream.pyleap0/common/code_interpreter.pyleap0/common/lsp.pyleap0/common/sandbox.pyleap0/common/snapshot.pyleap0/common/template.pyleap0/desktop.pyleap0/lsp.pytests/_utils/test_stream.pytests/common/test_lsp.pytests/common/test_template.pytests/test_import.py
✅ Files skipped from review due to trivial changes (5)
- tests/test_import.py
- tests/_utils/test_stream.py
- tests/common/test_template.py
- leap0/common/code_interpreter.py
- leap0/common/snapshot.py
🚧 Files skipped from review as they are similar to previous changes (5)
- leap0/_utils/stream.py
- tests/common/test_lsp.py
- leap0/desktop.py
- leap0/lsp.py
- leap0/common/sandbox.py
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
leap0/common/sandbox.py (1)
75-78:⚠️ Potential issue | 🟠 MajorValidate sandbox IDs consistently in both parsers.
Sandbox.from_dict()still accepts whitespace-only IDs, andSandboxStatus.from_dict()still manufacturesid=""on malformed payloads. Both flow throughsandbox_id_of(), so callers can end up building invalid sandbox URLs instead of getting a controlled parse error.💡 Suggested fix
def from_dict(cls, data: SandboxCreateResponseDict) -> Sandbox: sandbox_id = data.get("id") - if not sandbox_id or not isinstance(sandbox_id, str): + if not isinstance(sandbox_id, str) or not sandbox_id.strip(): raise ValueError(f"Sandbox response missing required non-empty string 'id', got: {sandbox_id!r}") + sandbox_id = sandbox_id.strip() state = _parse_sandbox_state(data.get("state")) return cls( id=sandbox_id, @@ def from_dict(cls, data: SandboxStatusResponseDict) -> SandboxStatus: + sandbox_id = data.get("id") + if not isinstance(sandbox_id, str) or not sandbox_id.strip(): + raise ValueError(f"Sandbox status response missing required non-empty string 'id', got: {sandbox_id!r}") + sandbox_id = sandbox_id.strip() state = _parse_sandbox_state(data.get("state")) return cls( - id=data.get("id", ""), + id=sandbox_id, template_id=data.get("template_id", ""), vcpu=int(data.get("vcpu", 0)),Also applies to: 105-108
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@leap0/common/sandbox.py` around lines 75 - 78, Sandbox.from_dict currently accepts whitespace-only IDs and SandboxStatus.from_dict can fabricate id="" on bad payloads, causing sandbox_id_of to receive invalid IDs and produce bad URLs; update both Sandbox.from_dict and SandboxStatus.from_dict to validate the id field the same way (ensure it's present, a str, and not empty or whitespace-only) and raise a ValueError on failure so callers get a controlled parse error instead of malformed ids flowing into sandbox_id_of.leap0/common/desktop.py (1)
243-250:⚠️ Potential issue | 🟡 MinorNormalize the
itemscontainer before iterating.The
isinstance(item, dict)guard only helps after iteration starts. If the API sends{"items": null},data.get("items", [])returnsNoneand this still raisesTypeError.💡 Suggested fix
`@classmethod` def from_dict(cls, data: DesktopProcessStatusListDict) -> DesktopProcessStatusList: + raw_items = data.get("items") + items = raw_items if isinstance(raw_items, list) else [] return cls( status=data.get("status", ""), items=[ DesktopProcessStatus.from_dict(item) # type: ignore[arg-type] - for item in data.get("items", []) + for item in items if isinstance(item, dict) ], running=_safe_int(data.get("running"), 0),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@leap0/common/desktop.py` around lines 243 - 250, In DesktopProcessStatusList.from_dict, guard against non-iterable/missing "items" by normalizing it before the list comprehension: fetch items = data.get("items") (or use an explicit default), then if items is None or not a list/set/tuple, replace it with an empty list; finally iterate over that normalized items and call DesktopProcessStatus.from_dict for dict entries only. This ensures DesktopProcessStatusList.from_dict and the DesktopProcessStatus.from_dict call don't raise TypeError when "items" is null or not a sequence.
🧹 Nitpick comments (1)
leap0/common/code_interpreter.py (1)
125-150: Consider consolidating duplicate base64 decoding logic.The three
*_bytes()methods share identical structure. A private helper would reduce duplication.♻️ Optional refactor to DRY up base64 decoding
+ def _decode_base64(self, data: str | None, field_name: str) -> bytes | None: + if not data: + return None + try: + return base64.b64decode(data) + except (binascii.Error, ValueError): + _logger.debug("Failed to decode %s base64 data", field_name) + return None + def png_bytes(self) -> bytes | None: - if not self.png: - return None - try: - return base64.b64decode(self.png) - except (binascii.Error, ValueError): - _logger.debug("Failed to decode png base64 data") - return None + return self._decode_base64(self.png, "png") def jpeg_bytes(self) -> bytes | None: - if not self.jpeg: - return None - try: - return base64.b64decode(self.jpeg) - except (binascii.Error, ValueError): - _logger.debug("Failed to decode jpeg base64 data") - return None + return self._decode_base64(self.jpeg, "jpeg") def pdf_bytes(self) -> bytes | None: - if not self.pdf: - return None - try: - return base64.b64decode(self.pdf) - except (binascii.Error, ValueError): - _logger.debug("Failed to decode pdf base64 data") - return None + return self._decode_base64(self.pdf, "pdf")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@leap0/common/code_interpreter.py` around lines 125 - 150, The three methods png_bytes, jpeg_bytes, and pdf_bytes duplicate base64 decode logic; add a private helper like _decode_base64(data: str | None, label: str) -> bytes | None that returns None if data is falsy, tries base64.b64decode(data) in a try/except catching (binascii.Error, ValueError), and logs failures with label to _logger.debug; then replace the bodies of png_bytes, jpeg_bytes, and pdf_bytes to simply call and return _decode_base64(self.png, "png"), _decode_base64(self.jpeg, "jpeg"), and _decode_base64(self.pdf, "pdf") respectively.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@leap0/common/config.py`:
- Around line 41-49: The __post_init__ method currently only checks .strip() as
a predicate but leaves surrounding whitespace in values; normalize api_key,
base_url, and sandbox_domain by calling .strip() on any provided value
(including values read from os.environ) before assigning them (use the stripped
env var or default constants DEFAULT_BASE_URL/DEFAULT_SANDBOX_DOMAIN when
appropriate) and then validate: if api_key is None or empty after stripping
raise ValueError, and for base_url/sandbox_domain assign the stripped
env/defaults when the stripped value is empty; update references to api_key,
base_url, and sandbox_domain in __post_init__ to use the normalized (stripped)
values.
In `@leap0/desktop.py`:
- Around line 387-408: The retry predicate on the _poll function currently
retries on all Leap0Error subclasses (via retry_if_exception_type((Leap0Error,
ConnectionError, OSError))), which causes permanent errors to be retried and
later masked as Leap0TimeoutError; change the retry condition used in the retry
decorator so it only retries transient transport errors (e.g.,
retry_if_exception_type((ConnectionError, OSError))) and/or a custom predicate
that only retries when a Leap0Error indicates a transient 5xx server error,
while allowing non-transient Leap0Error exceptions to propagate immediately;
keep reraise=True so permanent errors surface unchanged to the caller and adjust
the decorator on _poll accordingly.
---
Duplicate comments:
In `@leap0/common/desktop.py`:
- Around line 243-250: In DesktopProcessStatusList.from_dict, guard against
non-iterable/missing "items" by normalizing it before the list comprehension:
fetch items = data.get("items") (or use an explicit default), then if items is
None or not a list/set/tuple, replace it with an empty list; finally iterate
over that normalized items and call DesktopProcessStatus.from_dict for dict
entries only. This ensures DesktopProcessStatusList.from_dict and the
DesktopProcessStatus.from_dict call don't raise TypeError when "items" is null
or not a sequence.
In `@leap0/common/sandbox.py`:
- Around line 75-78: Sandbox.from_dict currently accepts whitespace-only IDs and
SandboxStatus.from_dict can fabricate id="" on bad payloads, causing
sandbox_id_of to receive invalid IDs and produce bad URLs; update both
Sandbox.from_dict and SandboxStatus.from_dict to validate the id field the same
way (ensure it's present, a str, and not empty or whitespace-only) and raise a
ValueError on failure so callers get a controlled parse error instead of
malformed ids flowing into sandbox_id_of.
---
Nitpick comments:
In `@leap0/common/code_interpreter.py`:
- Around line 125-150: The three methods png_bytes, jpeg_bytes, and pdf_bytes
duplicate base64 decode logic; add a private helper like _decode_base64(data:
str | None, label: str) -> bytes | None that returns None if data is falsy,
tries base64.b64decode(data) in a try/except catching (binascii.Error,
ValueError), and logs failures with label to _logger.debug; then replace the
bodies of png_bytes, jpeg_bytes, and pdf_bytes to simply call and return
_decode_base64(self.png, "png"), _decode_base64(self.jpeg, "jpeg"), and
_decode_base64(self.pdf, "pdf") respectively.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 373a5805-9816-497a-a73a-a7819e05729e
📒 Files selected for processing (17)
examples/desktop.pyleap0/__init__.pyleap0/_utils/errors.pyleap0/_utils/stream.pyleap0/common/code_interpreter.pyleap0/common/config.pyleap0/common/desktop.pyleap0/common/pty.pyleap0/common/sandbox.pyleap0/common/template.pyleap0/desktop.pyleap0/pty.pypyproject.tomltests/test_filesystem.pytests/test_process.pytests/test_ssh.pytests/test_templates.py
✅ Files skipped from review due to trivial changes (6)
- pyproject.toml
- tests/test_ssh.py
- leap0/common/template.py
- tests/test_templates.py
- tests/test_filesystem.py
- leap0/common/pty.py
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/test_process.py
- leap0/_utils/stream.py
- leap0/pty.py
- leap0/init.py
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
leap0/common/config.py (1)
50-62: Extract duplicated string-resolution logic to reduce drift.
base_urlandsandbox_domainuse near-identical normalization/fallback code. A small helper would make this easier to maintain and less error-prone.♻️ Refactor sketch
def __post_init__(self) -> None: + def _resolve_str(explicit: str | None, env_key: str, default: str) -> str: + explicit_clean = explicit.strip() if explicit else None + if explicit_clean: + return explicit_clean + env_val = os.environ.get(env_key) + env_clean = env_val.strip() if env_val else None + return env_clean or default + # Resolve api_key from env if not provided, then strip and validate. @@ - base_url = self.base_url.strip() if self.base_url else None - if not base_url: - env_base = os.environ.get("LEAP0_BASE_URL") - base_url = env_base.strip() if env_base else None - self.base_url = base_url or DEFAULT_BASE_URL + self.base_url = _resolve_str(self.base_url, "LEAP0_BASE_URL", DEFAULT_BASE_URL) @@ - sandbox_domain = self.sandbox_domain.strip() if self.sandbox_domain else None - if not sandbox_domain: - env_sd = os.environ.get("LEAP0_SANDBOX_DOMAIN") - sandbox_domain = env_sd.strip() if env_sd else None - self.sandbox_domain = sandbox_domain or DEFAULT_SANDBOX_DOMAIN + self.sandbox_domain = _resolve_str( + self.sandbox_domain, "LEAP0_SANDBOX_DOMAIN", DEFAULT_SANDBOX_DOMAIN + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@leap0/common/config.py` around lines 50 - 62, Extract the duplicated normalization/fallback logic into a small helper (e.g., _resolve_env_str or similar) that takes the provided value (like self.base_url or self.sandbox_domain), the environment variable name ("LEAP0_BASE_URL" / "LEAP0_SANDBOX_DOMAIN") and a default (DEFAULT_BASE_URL / DEFAULT_SANDBOX_DOMAIN), and returns the stripped value or the env/default fallback; then replace the two blocks that set self.base_url and self.sandbox_domain with calls to that helper (referencing self.base_url, "LEAP0_BASE_URL", DEFAULT_BASE_URL and self.sandbox_domain, "LEAP0_SANDBOX_DOMAIN", DEFAULT_SANDBOX_DOMAIN respectively). Ensure the helper handles None/empty strings and strips whitespace from both provided and env values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@leap0/common/config.py`:
- Around line 37-62: In __post_init__ validate the timeout attribute (timeout
and DEFAULT_CLIENT_TIMEOUT context) to ensure it is a positive, finite non-zero
float: convert int-like values if needed, reject None/0/negative/NaN/inf and
raise ValueError with a clear message; add this check near the top of the
__post_init__ method (before using timeout downstream) so invalid timeouts are
caught at config construction time.
In `@leap0/desktop.py`:
- Around line 362-365: The loop in which iter_sse_events(...) is consumed should
not raise Leap0Error for every str payload (which are heartbeat/info frames);
instead skip non-dict events and only error on an explicit error envelope —
update the loop that yields
DesktopProcessStatusList.from_dict(cast(DesktopProcessStatusListDict, event))
to: ignore or continue when event is a str (or not a dict), and only raise
Leap0Error when event is a dict containing a clear error marker (e.g., an
"error" key or whatever envelope your protocol uses); reference iter_sse_events,
DesktopProcessStatusList.from_dict, DesktopProcessStatusListDict and Leap0Error
when making this change.
- Around line 356-357: The retry in wait_until_ready is catching
ConnectionError/OSError but status_stream is wrapped by intercept_errors so
transport/connect failures are raised as Leap0Error/Leap0TimeoutError; update
the retry to catch Leap0Error and Leap0TimeoutError instead (import them from
leap0._utils.errors) for the wait_until_ready call used by status_stream and the
other affected uses (the call sites around the other ranges mentioned), so
transient SSE failures trigger reconnect retries rather than failing fast.
- Around line 391-401: The retry's stop_after_delay(timeout) doesn't interrupt
an active _poll() so you must enforce the timeout inside the SSE loop: record a
start time (time.monotonic()) at the start of _poll() and, on each iteration of
for status in self.status_stream(sandbox), check elapsed >= timeout and if so
raise a TimeoutError or Leap0Error to stop the loop; alternatively, if
status_stream accepts a timeout/deadline argument, pass the remaining time into
self.status_stream(sandbox, timeout=...) and raise when exhausted. Ensure the
raised exception type is covered by the retry/reraise behavior so the outer
retry honors the deadline.
---
Nitpick comments:
In `@leap0/common/config.py`:
- Around line 50-62: Extract the duplicated normalization/fallback logic into a
small helper (e.g., _resolve_env_str or similar) that takes the provided value
(like self.base_url or self.sandbox_domain), the environment variable name
("LEAP0_BASE_URL" / "LEAP0_SANDBOX_DOMAIN") and a default (DEFAULT_BASE_URL /
DEFAULT_SANDBOX_DOMAIN), and returns the stripped value or the env/default
fallback; then replace the two blocks that set self.base_url and
self.sandbox_domain with calls to that helper (referencing self.base_url,
"LEAP0_BASE_URL", DEFAULT_BASE_URL and self.sandbox_domain,
"LEAP0_SANDBOX_DOMAIN", DEFAULT_SANDBOX_DOMAIN respectively). Ensure the helper
handles None/empty strings and strips whitespace from both provided and env
values.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8e953f5c-2922-4e73-a76e-dbaaf5458c10
📒 Files selected for processing (6)
leap0/common/code_interpreter.pyleap0/common/config.pyleap0/common/desktop.pyleap0/common/sandbox.pyleap0/desktop.pytests/common/test_sandbox.py
✅ Files skipped from review due to trivial changes (1)
- leap0/common/code_interpreter.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/common/test_sandbox.py
- leap0/common/sandbox.py
Comment on lines
356
to
357
| @intercept_errors("Failed to stream status: ") | ||
| def status_stream(self, sandbox: SandboxRef) -> Iterator[DesktopProcessStatusList]: |
There was a problem hiding this comment.
wait_until_ready() is retrying the wrong exception types.
status_stream() is already wrapped by intercept_errors() in leap0/_utils/errors.py:38-69, so stream connect/read failures reach this retry decorator as Leap0Error / Leap0TimeoutError, not ConnectionError / OSError. That means transient SSE transport failures fail fast here instead of reconnecting.
Also applies to: 391-395, 398-398
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@leap0/desktop.py` around lines 356 - 357, The retry in wait_until_ready is
catching ConnectionError/OSError but status_stream is wrapped by
intercept_errors so transport/connect failures are raised as
Leap0Error/Leap0TimeoutError; update the retry to catch Leap0Error and
Leap0TimeoutError instead (import them from leap0._utils.errors) for the
wait_until_ready call used by status_stream and the other affected uses (the
call sites around the other ranges mentioned), so transient SSE failures trigger
reconnect retries rather than failing fast.
This was referenced Apr 1, 2026
Merged
Merged
Merged
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 by CodeRabbit
New Features
Bug Fixes
Breaking Changes
Tests