Refactor#12
Merged
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughSplit process output into stdout/stderr; introduced Pydantic request/response parameter models and stricter validation for desktop/filesystem/sandbox APIs; tightened multipart and SSE parsing/validation; forwarded optional http_timeout for sandbox pause; updated examples and tests to match new shapes and behaviors. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
leap0/_sync/filesystem.py (1)
508-513:⚠️ Potential issue | 🟠 MajorInconsistent error handling: sync version exposes body preview while async version redacts it.
The sync version includes
preview={body_preview!r}in the error message (up to 200 bytes of the response body), while the async version inleap0/_async/filesystem.pyusespreview='<redacted>'. This inconsistency could lead to sensitive data exposure in error logs or messages for the sync client.🔒 Suggested fix to redact preview for consistency
if not msg.is_multipart(): - body_preview = body[:200] if len(body) > 200 else body raise ValueError( f"Expected multipart response but got content_type={content_type!r} " - f"(body length={len(body)}, preview={body_preview!r})" + f"(body length={len(body)}, preview='<redacted>')" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@leap0/_sync/filesystem.py` around lines 508 - 513, The sync branch currently includes a body preview in the ValueError raised when a non-multipart response is received (see the check using msg.is_multipart(), variables body/body_preview, and the raise ValueError call); change the error message to redact the preview (e.g., use preview='<redacted>') to match the async implementation and avoid exposing sensitive response content, leaving other context (content_type and body length) intact.leap0/_sync/desktop.py (1)
637-644:⚠️ Potential issue | 🟠 MajorThe structured SSE error branch is currently shadowed.
The generic
if "error" in eventbranch runs before the new{error, message}validation, so structured server errors never useDesktopStatusStreamErrorEvent.detail. Because this condition usesintersection(), its remaining reachable case is mostlymessage-only payloads.🛠️ Suggested control-flow fix
- if "error" in event: - raise Leap0Error( - "Desktop status stream error", - body=str(event["error"]), - ) - if {"error", "message"}.intersection(event) and not {"status", "items", "running", "total"}.intersection(event): + if {"error", "message"} <= event.keys() and not {"status", "items", "running", "total"}.intersection(event): error_event = DesktopStatusStreamErrorEvent.model_validate(event) raise Leap0Error("Desktop status stream error", body=error_event.detail) + if "error" in event: + raise Leap0Error( + "Desktop status stream error", + body=str(event["error"]), + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@leap0/_sync/desktop.py` around lines 637 - 644, The generic check if "error" in event is shadowing the structured SSE error branch; update the control flow in the Desktop status stream processing so you first detect and handle structured errors via DesktopStatusStreamErrorEvent.model_validate(event) (the branch that checks if {"error","message"}.intersection(event) and not {"status","items","running","total"}.intersection(event)) and raise Leap0Error with error_event.detail, and only after that fall back to the simple if "error" in event branch that raises Leap0Error with body=str(event["error"]); this ensures structured server errors are validated and use DesktopStatusStreamErrorEvent.detail instead of being caught by the generic handler.
🧹 Nitpick comments (9)
leap0/models/snapshot.py (1)
81-92: Validation usesstrip()but stores the original value.The validation checks
snapshot_id.strip()andsnapshot_name.strip()for emptiness, but the unstripped values are stored. This differs fromCreateSnapshotParams(lines 20-25) which strips before storing. If this inconsistency is intentional (preserve exact API response), consider adding a brief comment. Otherwise, consider stripping consistently:♻️ Optional: Strip values for consistency
snapshot_id = data.get("id") - if not isinstance(snapshot_id, str) or not snapshot_id.strip(): + if not isinstance(snapshot_id, str) or not (snapshot_id := snapshot_id.strip()): raise ValueError(f"Snapshot response missing required non-empty string 'id', got: {snapshot_id!r}") snapshot_name = data.get("name") - if not isinstance(snapshot_name, str) or not snapshot_name.strip(): + if not isinstance(snapshot_name, str) or not (snapshot_name := snapshot_name.strip()): raise ValueError( f"Snapshot response missing required non-empty string 'name', got: {snapshot_name!r}" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@leap0/models/snapshot.py` around lines 81 - 92, Validation currently calls snapshot_id.strip() and snapshot_name.strip() but then passes the original unstripped values into cls(...), causing inconsistency with CreateSnapshotParams; update the code to use the stripped values when constructing the instance (e.g., set id=snapshot_id.strip() and name=snapshot_name.strip()) so stored values match validation, and adjust any references to snapshot_id/snapshot_name accordingly (alternatively, if preserving original API response is intentional, add a concise comment above the checks explaining why the raw values are preserved).tests/models/test_snapshot.py (1)
32-38: Consider adding edge case tests for validation.The validation in
Snapshot.from_dicthandles empty strings, whitespace-only strings, and non-string types, but only the missing-key case is tested here.🧪 Optional: Additional test cases
def test_from_dict_rejects_empty_id(self): with pytest.raises(ValueError, match="Snapshot response missing required non-empty string 'id'"): Snapshot.from_dict({"id": "", "name": "my-snap"}) def test_from_dict_rejects_whitespace_only_id(self): with pytest.raises(ValueError, match="Snapshot response missing required non-empty string 'id'"): Snapshot.from_dict({"id": " ", "name": "my-snap"}) def test_from_dict_rejects_non_string_id(self): with pytest.raises(ValueError, match="Snapshot response missing required non-empty string 'id'"): Snapshot.from_dict({"id": 123, "name": "my-snap"})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/models/test_snapshot.py` around lines 32 - 38, Add edge-case tests in tests/models/test_snapshot.py to validate Snapshot.from_dict rejects empty, whitespace-only, and non-string values for required fields; specifically add tests like test_from_dict_rejects_empty_id, test_from_dict_rejects_whitespace_only_id, test_from_dict_rejects_non_string_id (and mirror equivalents for name) that call Snapshot.from_dict and assert pytest.raises(ValueError, match="Snapshot response missing required non-empty string 'id'") or the matching message for 'name' as appropriate to ensure the validator in Snapshot.from_dict correctly rejects those inputs.tests/_sync/test_filesystem.py (1)
53-67: Inconsistent exception type assertion with async tests.The async tests use
pytest.raises(Leap0Error, match=...)while these sync tests use genericpytest.raises(Exception, match=...). Since both clients use the@intercept_errorsdecorator (which convertsValidationErrortoLeap0Error), these tests should also assertLeap0Errorfor consistency and to ensure the correct error type is raised.♻️ Suggested fix for consistency
+from leap0.models.errors import Leap0Error + class TestFilesystemClient: ... def test_read_bytes_rejects_head_and_tail(self, mock_transport): - with pytest.raises(Exception, match="mutually exclusive"): + with pytest.raises(Leap0Error, match="mutually exclusive"): FilesystemClient(mock_transport).read_bytes("sbx-1", path="/workspace/hello.bin", head=1, tail=1) def test_set_permissions_rejects_missing_or_blank_updates(self, mock_transport): client = FilesystemClient(mock_transport) - with pytest.raises(Exception, match="at least one of mode, owner, or group"): + with pytest.raises(Leap0Error, match="at least one of mode, owner, or group"): client.set_permissions("sbx-1", path="/workspace/a.txt") - with pytest.raises(Exception, match="mode must be a non-empty string"): + with pytest.raises(Leap0Error, match="mode must be a non-empty string"): client.set_permissions("sbx-1", path="/workspace/a.txt", mode=" ") - with pytest.raises(Exception, match="owner must be a non-empty string"): + with pytest.raises(Leap0Error, match="owner must be a non-empty string"): client.set_permissions("sbx-1", path="/workspace/a.txt", owner="")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/_sync/test_filesystem.py` around lines 53 - 67, Update the sync tests to assert Leap0Error instead of generic Exception so they match the async tests and the `@intercept_errors` behavior; specifically change the pytest.raises in test_read_bytes_rejects_head_and_tail (the call to FilesystemClient(mock_transport).read_bytes(..., head=1, tail=1)) and the three pytest.raises in test_set_permissions_rejects_missing_or_blank_updates (calls to client.set_permissions(...), including the blank mode and owner cases) to use pytest.raises(Leap0Error, match=...) so ValidationError conversions via intercept_errors are properly asserted.leap0/_sync/filesystem.py (2)
516-528: Same variable shadowing issue as async version.Line 518 shadows the
content_typefunction parameter. Apply the same rename topart_content_typeas suggested for the async version.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@leap0/_sync/filesystem.py` around lines 516 - 528, The local variable content_type in the /read-files parsing block shadows the function parameter content_type; rename the local variable (e.g., to part_content_type) and update all usages in this block (the comparison against "application/octet-stream" and the two ValueError messages) as well as the subsequent payload check so the parameter name is preserved and no shadowing occurs in the function that contains this code (look for the parsing loop handling multipart parts and the variables part, payload, and result).
501-529: Extract_parse_multipart_responseto a shared utility module.The function exists in both
leap0/_sync/filesystem.pyandleap0/_async/filesystem.pywith near-identical implementations. The only difference is error message formatting in theis_multipart()check: async uses a hardcoded'<redacted>'while sync generates a body preview. Extracting to a shared module (e.g.,leap0/_internal/multipart.py) would eliminate duplication. The preview difference can be handled with a parameter or harmonized across both implementations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@leap0/_sync/filesystem.py` around lines 501 - 529, Extract the duplicated _parse_multipart_response implementation into a single shared utility (e.g., create a module named _internal.multipart and export parse_multipart_response) and replace the copies in both _parse_multipart_response implementations with imports from that module; add a boolean parameter (e.g., redact_preview: bool = False) to the shared function to switch between showing a body preview or returning a redacted placeholder so callers in sync and async can preserve their current error-message behavior, and update both call sites to import and call parse_multipart_response(name=...) with the appropriate redact_preview value.leap0/_async/filesystem.py (1)
515-527: Variablecontent_typeshadows function parameter.Line 517 reassigns
content_typewhich shadows the function parameter of the same name. While functionally correct here (the parameter isn't used after the loop starts), this is confusing and could cause issues if the code is modified later.♻️ Rename to avoid shadowing
for part in msg.get_payload(): # type: ignore[union-attr] name = part.get_param("name", header="content-disposition") if not name: continue - content_type = part.get_content_type() - if content_type != "application/octet-stream": + part_content_type = part.get_content_type() + if part_content_type != "application/octet-stream": raise ValueError( - f"Failed to parse /read-files response: expected file bytes for entry {name!r}, got {content_type}" + f"Failed to parse /read-files response: expected file bytes for entry {name!r}, got {part_content_type}" ) payload = part.get_payload(decode=True) if payload is None: raise ValueError( - f"Failed to parse /read-files response: expected file bytes for entry {name!r}, got {content_type}" + f"Failed to parse /read-files response: expected file bytes for entry {name!r}, got {part_content_type}" ) result[str(name)] = payload🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@leap0/_async/filesystem.py` around lines 515 - 527, The loop-local variable content_type defined as part.get_content_type() shadows the function parameter content_type; rename the local variable (for example to part_content_type or entry_content_type) and update its usages in the surrounding block (the equality check and the error messages) so the parameter name is no longer shadowed and intent is clear.tests/_utils/test_stream.py (1)
38-38: Add a JSONevent: errorregression test to lock parser behavior.Line 38 covers plain-text error events well; please also add a case like
data: {"error":"boom"}so structured error payloads are protected from accidental stringification.Suggested additional test
class TestIterSseEvents: @@ def test_plain_text_data_preserved(self): assert list(iter_sse_events(["event: error", "data: desktop stream failed", ""])) == [{"error": "desktop stream failed"}] + + def test_error_event_with_json_payload_preserved(self): + assert list(iter_sse_events(["event: error", 'data: {"error":"boom"}', ""])) == [{"error": "boom"}]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/_utils/test_stream.py` at line 38, Add a regression test to ensure JSON payloads for error SSE events are parsed rather than stringified: in tests/_utils/test_stream.py add another assertion calling iter_sse_events with an event block like ["event: error", "data: {\"error\":\"boom\"}", ""] and assert the result equals [{"error":"boom"}]; keep the existing plain-text error test as-is and reference iter_sse_events so the parser preserves structured error payloads.leap0/models/desktop.py (2)
317-332: Non-dict items initemsarray are silently discarded.The validation at line 321-322 correctly checks that
itemsis a list/tuple, but the list comprehension at lines 326-328 silently skips any elements that aren't dicts. If the API returns malformed data (e.g.,nullor string elements in the array), this will silently produce incomplete results rather than raising an error.Consider whether this is intentional (defensive parsing) or if you'd prefer to fail fast on malformed elements:
💡 Optional stricter validation
return cls( status=_require_str(data, "status"), items=[ DesktopProcessStatus.from_dict(item) # type: ignore[arg-type] for item in raw_items - if isinstance(item, dict) ], running=_require_int(data, "running"), total=_require_int(data, "total"), )This would raise a
TypeErroron non-dict items, making malformed data visible rather than silently filtered.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@leap0/models/desktop.py` around lines 317 - 332, The current DesktopProcessStatusList.from_dict silently skips non-dict entries in data["items"]; instead validate each element and fail fast: iterate raw_items, for each index ensure isinstance(item, dict) and if not raise a TypeError (include the index and offending value), otherwise call DesktopProcessStatus.from_dict(item); keep using _require_str/_require_int for other fields to preserve existing validations.
86-102: Consider validating thebuttonparameter.The
x/ypairing validation is correct. However,buttonaccepts any integer without validation. If the API only accepts specific values (e.g., 1=left, 2=middle, 3=right), consider adding bounds validation similar to other fields.💡 Optional validation for button
`@model_validator`(mode="after") def _validate_click(self) -> "DesktopClickParams": if (self.x is None) != (self.y is None): raise ValueError("x and y must be provided together or both omitted") for name, value in (("x", self.x), ("y", self.y)): if value is not None and value < 0: raise ValueError(f"{name} must be >= 0") + if self.button is not None and self.button not in {1, 2, 3}: + raise ValueError("button must be 1 (left), 2 (middle), or 3 (right)") return self🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@leap0/models/desktop.py` around lines 86 - 102, The _validate_click validator currently checks x/y but not button; update DesktopClickParams._validate_click to also validate the button field (e.g., ensure self.button is either None or one of the allowed values such as 1,2,3 or within an allowed range like 1..3) and raise a ValueError with a clear message when it’s out of bounds; modify the existing for-loop/validator body in DesktopClickParams to perform this extra check after x/y validation so invalid button values are rejected.
🤖 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/_async/desktop.py`:
- Around line 573-577: The generic "if 'error' in event" check currently runs
before the structured error handling and prevents
DesktopStatusStreamErrorEvent.model_validate from ever being reached; move the
structured branch that calls DesktopStatusStreamErrorEvent.model_validate(...)
above the simple "error" key check, and adjust the condition to detect the
structured SSE form (e.g., if {"error","message"}.issubset(event) or at least if
"message" in event and "error" in event and not
{"status","items","running","total"}.intersection(event)) so that the code
raises Leap0Error(..., body=error_event.detail) for structured events and falls
back to the generic raise Leap0Error("Desktop status stream error",
body=str(event["error"])) only afterwards.
In `@leap0/_utils/stream.py`:
- Around line 43-45: The current branch that handles event_name == "error"
returns {"error": data} which forces raw string wrapping and loses JSON
structure; change this to attempt to parse the SSE payload using
_parse_sse_data(data) and, if parsing yields a structured value (e.g.,
dict/list), return {"error": parsed_value}, otherwise fall back to {"error":
data}; update the conditional in the event handling branch (event_name ==
"error") to call _parse_sse_data and use the parsed result when structured.
In `@leap0/models/config.py`:
- Around line 40-41: The current logic in the config where it falls back to
os.environ.get(OTEL_EXPORTER_OTLP_ENDPOINT_ENV) (when sdk_otel_env is falsy)
treats whitespace-only values as enabled; change the fallback to read the env
var, strip whitespace, and only return True if the stripped value is non-empty
(e.g., compute endpoint = os.environ.get(OTEL_EXPORTER_OTLP_ENDPOINT_ENV) and
return bool(endpoint and endpoint.strip())); keep the existing sdk_otel_env
check and only apply this stricter validation when sdk_otel_env is falsy.
In `@leap0/models/process.py`:
- Around line 10-11: The deserializer no longer accepts the legacy "result"
payload, so update ProcessResult.from_dict (and any deserialization helpers) to
detect and accept a legacy "result" key: when "result" is present, map its
contents into the new fields (populate stdout and/or stderr appropriately or
fallback to empty strings) and preserve backward compatibility; also keep
ProcessResult.result access (or provide a compatibility property) that returns
the combined/result value so older callers continue to work during rollout.
In `@leap0/models/sandbox.py`:
- Around line 82-88: The loop over transforms in sandbox.py can raise a KeyError
when a transform dict lacks the "domain" key; update the loop in the transforms
handling (the block that calls _validate_domain_pattern) to validate that each
transform is a mapping and contains the "domain" key and, if missing, raise a
descriptive ValueError (include the transform index or content in the message)
instead of accessing transform["domain"] directly; then pass the validated value
to _validate_domain_pattern for further validation.
In `@tests/_async/test_desktop.py`:
- Around line 30-31: The test is asserting on the unused
async_mock_transport.request mock which won't catch HTTP calls made via
request_target; change the first assertion to assert
async_mock_transport.request_target.call_count == 0 so screenshot() /
screenshot_region() paths (which go through _request() → request_target()) are
properly covered; keep the existing assertion for
async_mock_transport.request_target_json.call_count == 0 so both request_target
and request_target_json are verified.
In `@tests/_sync/test_desktop.py`:
- Around line 28-29: The test is asserting the unused mock_transport.request
instead of the transport method actually invoked by
screenshot()/screenshot_region(); change the first assertion to assert on
mock_transport.request_target.call_count == 0 (keeping the existing assert for
mock_transport.request_target_json.call_count == 0) so the test verifies that
_request() → request_target() was not called; update references to
mock_transport.request in this test to mock_transport.request_target to
correctly catch regressions related to request_target() validation.
In `@tests/models/test_desktop.py`:
- Around line 66-67: Replace the hard-coded /tmp paths used as string fixtures
for "stdout_log" and "stderr_log" in tests/models/test_desktop.py (the dict
entries for stdout_log and stderr_log around the blocks you changed and the
other occurrences at the same file) with a neutral placeholder path (e.g.
"placeholder/xvfb.stdout.log" and "placeholder/xvfb.stderr.log" or similar
non-absolute names) so Ruff S108 won't flag them; update all three occurrence
pairs (the ones you noted at lines ~66-67, ~83-84, ~108-109) so the tests still
pass while avoiding hardcoded temp directories.
---
Outside diff comments:
In `@leap0/_sync/desktop.py`:
- Around line 637-644: The generic check if "error" in event is shadowing the
structured SSE error branch; update the control flow in the Desktop status
stream processing so you first detect and handle structured errors via
DesktopStatusStreamErrorEvent.model_validate(event) (the branch that checks if
{"error","message"}.intersection(event) and not
{"status","items","running","total"}.intersection(event)) and raise Leap0Error
with error_event.detail, and only after that fall back to the simple if "error"
in event branch that raises Leap0Error with body=str(event["error"]); this
ensures structured server errors are validated and use
DesktopStatusStreamErrorEvent.detail instead of being caught by the generic
handler.
In `@leap0/_sync/filesystem.py`:
- Around line 508-513: The sync branch currently includes a body preview in the
ValueError raised when a non-multipart response is received (see the check using
msg.is_multipart(), variables body/body_preview, and the raise ValueError call);
change the error message to redact the preview (e.g., use preview='<redacted>')
to match the async implementation and avoid exposing sensitive response content,
leaving other context (content_type and body length) intact.
---
Nitpick comments:
In `@leap0/_async/filesystem.py`:
- Around line 515-527: The loop-local variable content_type defined as
part.get_content_type() shadows the function parameter content_type; rename the
local variable (for example to part_content_type or entry_content_type) and
update its usages in the surrounding block (the equality check and the error
messages) so the parameter name is no longer shadowed and intent is clear.
In `@leap0/_sync/filesystem.py`:
- Around line 516-528: The local variable content_type in the /read-files
parsing block shadows the function parameter content_type; rename the local
variable (e.g., to part_content_type) and update all usages in this block (the
comparison against "application/octet-stream" and the two ValueError messages)
as well as the subsequent payload check so the parameter name is preserved and
no shadowing occurs in the function that contains this code (look for the
parsing loop handling multipart parts and the variables part, payload, and
result).
- Around line 501-529: Extract the duplicated _parse_multipart_response
implementation into a single shared utility (e.g., create a module named
_internal.multipart and export parse_multipart_response) and replace the copies
in both _parse_multipart_response implementations with imports from that module;
add a boolean parameter (e.g., redact_preview: bool = False) to the shared
function to switch between showing a body preview or returning a redacted
placeholder so callers in sync and async can preserve their current
error-message behavior, and update both call sites to import and call
parse_multipart_response(name=...) with the appropriate redact_preview value.
In `@leap0/models/desktop.py`:
- Around line 317-332: The current DesktopProcessStatusList.from_dict silently
skips non-dict entries in data["items"]; instead validate each element and fail
fast: iterate raw_items, for each index ensure isinstance(item, dict) and if not
raise a TypeError (include the index and offending value), otherwise call
DesktopProcessStatus.from_dict(item); keep using _require_str/_require_int for
other fields to preserve existing validations.
- Around line 86-102: The _validate_click validator currently checks x/y but not
button; update DesktopClickParams._validate_click to also validate the button
field (e.g., ensure self.button is either None or one of the allowed values such
as 1,2,3 or within an allowed range like 1..3) and raise a ValueError with a
clear message when it’s out of bounds; modify the existing for-loop/validator
body in DesktopClickParams to perform this extra check after x/y validation so
invalid button values are rejected.
In `@leap0/models/snapshot.py`:
- Around line 81-92: Validation currently calls snapshot_id.strip() and
snapshot_name.strip() but then passes the original unstripped values into
cls(...), causing inconsistency with CreateSnapshotParams; update the code to
use the stripped values when constructing the instance (e.g., set
id=snapshot_id.strip() and name=snapshot_name.strip()) so stored values match
validation, and adjust any references to snapshot_id/snapshot_name accordingly
(alternatively, if preserving original API response is intentional, add a
concise comment above the checks explaining why the raw values are preserved).
In `@tests/_sync/test_filesystem.py`:
- Around line 53-67: Update the sync tests to assert Leap0Error instead of
generic Exception so they match the async tests and the `@intercept_errors`
behavior; specifically change the pytest.raises in
test_read_bytes_rejects_head_and_tail (the call to
FilesystemClient(mock_transport).read_bytes(..., head=1, tail=1)) and the three
pytest.raises in test_set_permissions_rejects_missing_or_blank_updates (calls to
client.set_permissions(...), including the blank mode and owner cases) to use
pytest.raises(Leap0Error, match=...) so ValidationError conversions via
intercept_errors are properly asserted.
In `@tests/_utils/test_stream.py`:
- Line 38: Add a regression test to ensure JSON payloads for error SSE events
are parsed rather than stringified: in tests/_utils/test_stream.py add another
assertion calling iter_sse_events with an event block like ["event: error",
"data: {\"error\":\"boom\"}", ""] and assert the result equals
[{"error":"boom"}]; keep the existing plain-text error test as-is and reference
iter_sse_events so the parser preserves structured error payloads.
In `@tests/models/test_snapshot.py`:
- Around line 32-38: Add edge-case tests in tests/models/test_snapshot.py to
validate Snapshot.from_dict rejects empty, whitespace-only, and non-string
values for required fields; specifically add tests like
test_from_dict_rejects_empty_id, test_from_dict_rejects_whitespace_only_id,
test_from_dict_rejects_non_string_id (and mirror equivalents for name) that call
Snapshot.from_dict and assert pytest.raises(ValueError, match="Snapshot response
missing required non-empty string 'id'") or the matching message for 'name' as
appropriate to ensure the validator in Snapshot.from_dict correctly rejects
those inputs.
🪄 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: 00f00c6b-62a0-4327-9a61-c76c70717011
📒 Files selected for processing (33)
examples/async_quickstart.pyexamples/quickstart.pyleap0/_async/desktop.pyleap0/_async/filesystem.pyleap0/_async/process.pyleap0/_async/sandbox.pyleap0/_schemas/desktop.pyleap0/_schemas/process.pyleap0/_sync/desktop.pyleap0/_sync/filesystem.pyleap0/_sync/process.pyleap0/_sync/sandbox.pyleap0/_utils/stream.pyleap0/models/config.pyleap0/models/desktop.pyleap0/models/filesystem.pyleap0/models/process.pyleap0/models/sandbox.pyleap0/models/snapshot.pytests/_async/test_desktop.pytests/_async/test_filesystem.pytests/_async/test_process.pytests/_async/test_sandboxes.pytests/_sync/test_client_config.pytests/_sync/test_desktop.pytests/_sync/test_filesystem.pytests/_sync/test_process.pytests/_sync/test_sandboxes.pytests/_utils/test_stream.pytests/models/test_desktop.pytests/models/test_process.pytests/models/test_sandbox.pytests/models/test_snapshot.py
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
leap0/_sync/filesystem.py (1)
502-503: Wrapper function is now a simple pass-through.The local
_parse_multipart_responsefunction just delegates to the shared utility. Consider importing and usingparse_multipart_responsedirectly at line 280 to eliminate the indirection, unless you're preserving backward compatibility for internal callers.♻️ Simplify by using the import directly
def read_files_bytes( ... ) -> dict[str, bytes]: ... response = self._transport.request(...) - return _parse_multipart_response(response.headers.get("content-type", ""), response.content) + return parse_multipart_response(response.headers.get("content-type", ""), response.content) -def _parse_multipart_response(content_type: str, body: bytes) -> dict[str, bytes]: - return parse_multipart_response(content_type, body)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@leap0/_sync/filesystem.py` around lines 502 - 503, The local wrapper function _parse_multipart_response simply forwards to parse_multipart_response; remove the wrapper and update all internal call sites to call the imported parse_multipart_response directly (replace uses of _parse_multipart_response with parse_multipart_response) and delete the redundant _parse_multipart_response definition to remove the indirection while keeping the existing import of parse_multipart_response.leap0/_utils/multipart.py (1)
22-29: Hardcoded error message couples utility to specific use case.The error messages reference
/read-files response, but this is a general-purpose utility function that could be reused elsewhere. Consider parameterizing the context description or using a generic message.♻️ Suggested improvement
-def parse_multipart_response(content_type: str, body: bytes) -> dict[str, bytes]: +def parse_multipart_response(content_type: str, body: bytes, *, context: str = "multipart response") -> dict[str, bytes]: raw = f"Content-Type: {content_type}\r\n\r\n".encode() + body msg = BytesParser().parsebytes(raw) result: dict[str, bytes] = {} if not msg.is_multipart(): raise ValueError( f"Expected multipart response but got content_type={content_type!r} " f"(body length={len(body)}, preview='<redacted>')" ) for part in msg.get_payload(): # type: ignore[union-attr] name = part.get_param("name", header="content-disposition") if not name: continue part_content_type = part.get_content_type() if part_content_type != "application/octet-stream": raise ValueError( - f"Failed to parse /read-files response: expected file bytes for entry {name!r}, got {part_content_type}" + f"Failed to parse {context}: expected file bytes for entry {name!r}, got {part_content_type}" ) payload = part.get_payload(decode=True) if payload is None: raise ValueError( - f"Failed to parse /read-files response: expected file bytes for entry {name!r}, got {part_content_type}" + f"Failed to parse {context}: expected file bytes for entry {name!r}, got {part_content_type}" ) result[str(name)] = payload return result🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@leap0/_utils/multipart.py` around lines 22 - 29, The error messages in this multipart parsing utility are hardcoded to "/read-files response"; update the function (the routine that checks part_content_type and payload—look for uses of part_content_type, payload, and part.get_payload) to accept an optional context string parameter (e.g., context="multipart response") or default to a generic phrase, and use that context in the two ValueError messages instead of the literal "/read-files response" so the utility can be reused without coupling to that endpoint.
🤖 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/_async/desktop.py`:
- Around line 574-584: The inner except block catches ValidationError after
attempting DesktopStatusStreamErrorEvent.model_validate(event) but the fallback
raise for the generic "error" key doesn't chain the original status_error;
update the branch inside the ValidationError handler so the generic error path
uses "raise Leap0Error('Desktop status stream error', body=str(event['error']))
from status_error" (referencing DesktopStatusStreamErrorEvent.model_validate,
DesktopProcessStatusList.from_dict and the local status_error variable) so the
original TypeError/ValueError is chained.
In `@leap0/_sync/desktop.py`:
- Around line 637-650: In the Desktop status stream parsing block, ensure
exception chaining is consistent: when ValidationError catches and the event
contains "error", raise Leap0Error(...) chained to the original status_error
(use "raise Leap0Error(...) from status_error") and when re-raising the original
parsing error use a bare "raise" instead of "raise status_error" so the original
traceback is preserved; update the code paths around
DesktopProcessStatusList.from_dict and
DesktopStatusStreamErrorEvent.model_validate to apply these changes.
In `@leap0/models/sandbox.py`:
- Around line 70-72: The validation currently calls
_validate_domain_pattern(domain) but discards its normalized/trimmed return
value, so update the code to capture and persist the normalized domains: replace
or rebuild allow_domains with the values returned by _validate_domain_pattern
for each entry (and do the same for the similar loop handling deny_domains
around the other occurrence). Ensure you assign the returned trimmed string back
into the list or construct a new list of normalized domains so validated domains
used later are the normalized values.
In `@tests/models/test_sandbox.py`:
- Line 59: The test's pytest.raises(match=...) uses the literal
"network_policy.mode" but the dot is an unescaped regex metacharacter; update
the pytest.raises call (the match argument in the test that uses pytest.raises)
to escape the dot (e.g., use "network_policy\\.mode" or construct the pattern
via re.escape("network_policy.mode")) so the assertion matches the literal
token; keep the rest of the test unchanged.
---
Nitpick comments:
In `@leap0/_sync/filesystem.py`:
- Around line 502-503: The local wrapper function _parse_multipart_response
simply forwards to parse_multipart_response; remove the wrapper and update all
internal call sites to call the imported parse_multipart_response directly
(replace uses of _parse_multipart_response with parse_multipart_response) and
delete the redundant _parse_multipart_response definition to remove the
indirection while keeping the existing import of parse_multipart_response.
In `@leap0/_utils/multipart.py`:
- Around line 22-29: The error messages in this multipart parsing utility are
hardcoded to "/read-files response"; update the function (the routine that
checks part_content_type and payload—look for uses of part_content_type,
payload, and part.get_payload) to accept an optional context string parameter
(e.g., context="multipart response") or default to a generic phrase, and use
that context in the two ValueError messages instead of the literal "/read-files
response" so the utility can be reused without coupling to that endpoint.
🪄 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: 81229321-f3a5-4061-9a3a-462948d59c9d
📒 Files selected for processing (21)
leap0/_async/desktop.pyleap0/_async/filesystem.pyleap0/_schemas/process.pyleap0/_sync/desktop.pyleap0/_sync/filesystem.pyleap0/_utils/multipart.pyleap0/_utils/stream.pyleap0/models/config.pyleap0/models/desktop.pyleap0/models/process.pyleap0/models/sandbox.pyleap0/models/snapshot.pytests/_async/test_desktop.pytests/_sync/test_client_config.pytests/_sync/test_desktop.pytests/_sync/test_filesystem.pytests/_utils/test_stream.pytests/models/test_desktop.pytests/models/test_process.pytests/models/test_sandbox.pytests/models/test_snapshot.py
✅ Files skipped from review due to trivial changes (2)
- tests/_sync/test_client_config.py
- tests/_async/test_desktop.py
🚧 Files skipped from review as they are similar to previous changes (10)
- tests/models/test_process.py
- tests/_utils/test_stream.py
- leap0/_schemas/process.py
- leap0/models/config.py
- tests/models/test_snapshot.py
- leap0/_async/filesystem.py
- tests/_sync/test_filesystem.py
- tests/models/test_desktop.py
- leap0/models/process.py
- leap0/models/desktop.py
Comment on lines
+574
to
+584
| try: | ||
| yield DesktopProcessStatusList.from_dict(cast(DesktopProcessStatusListDict, event)) | ||
| continue | ||
| if "error" in event: | ||
| raise Leap0Error("Desktop status stream error", body=str(event["error"])) | ||
| yield DesktopProcessStatusList.from_dict(cast(DesktopProcessStatusListDict, event)) | ||
| except (TypeError, ValueError) as status_error: | ||
| try: | ||
| error_event = DesktopStatusStreamErrorEvent.model_validate(event) | ||
| except ValidationError: | ||
| if "error" in event: | ||
| raise Leap0Error("Desktop status stream error", body=str(event["error"])) from status_error | ||
| raise status_error | ||
| raise Leap0Error("Desktop status stream error", body=error_event.detail) from status_error |
There was a problem hiding this comment.
The structured SSE error branch logic is now correctly ordered.
The refactored flow attempts DesktopStatusStreamErrorEvent.model_validate() before falling back to the generic "error" key check, addressing the previous shadowing concern.
However, per static analysis (Ruff B904), Line 583 should use raise ... from to properly chain exceptions:
Proposed fix for exception chaining
except ValidationError:
if "error" in event:
raise Leap0Error("Desktop status stream error", body=str(event["error"])) from status_error
- raise status_error
+ raise status_error from None
raise Leap0Error("Desktop status stream error", body=error_event.detail) from status_error🧰 Tools
🪛 Ruff (0.15.9)
[warning] 583-583: 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 the current code and only fix it if needed.
In `@leap0/_async/desktop.py` around lines 574 - 584, The inner except block
catches ValidationError after attempting
DesktopStatusStreamErrorEvent.model_validate(event) but the fallback raise for
the generic "error" key doesn't chain the original status_error; update the
branch inside the ValidationError handler so the generic error path uses "raise
Leap0Error('Desktop status stream error', body=str(event['error'])) from
status_error" (referencing DesktopStatusStreamErrorEvent.model_validate,
DesktopProcessStatusList.from_dict and the local status_error variable) so the
original TypeError/ValueError is chained.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
leap0/models/sandbox.py (2)
37-40:⚠️ Potential issue | 🟡 MinorAdd a runtime type guard before
.strip()Line 38 assumes
valueis a string. If malformed input slips through (e.g.,123inallow_domains), this raisesAttributeErrorinstead of the validator’s expectedValueError.Proposed fix
def _validate_domain_pattern(value: str) -> str: + if not isinstance(value, str): + raise ValueError(f"invalid network policy domain pattern: {value!r}") domain = value.strip() if not domain: raise ValueError("network policy domains must be non-empty")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@leap0/models/sandbox.py` around lines 37 - 40, The validator _validate_domain_pattern assumes value is a str and calls value.strip(), which can raise AttributeError for non-strings; add a runtime type guard at the start of _validate_domain_pattern (e.g., check isinstance(value, str)) and if the check fails raise ValueError("network policy domains must be non-empty") or a clearer ValueError indicating invalid type, then proceed to call strip() and the existing empty check so malformed inputs (e.g., integers in allow_domains) produce the expected ValueError.
86-95:⚠️ Potential issue | 🟡 MinorDon’t mutate values after accepting generic
MappingLines 87-94 accept any
Mapping, but Line 94 writes to it. Immutable mappings will fail withTypeError. Either requireMutableMappingor normalize todictbefore mutation.Proposed fix (normalize transforms)
- for index, transform in enumerate(transforms): + normalized_transforms = [] + for index, transform in enumerate(transforms): if not isinstance(transform, Mapping): raise ValueError(f"network_policy.transforms[{index}] must be a mapping, got: {transform!r}") domain = transform.get("domain") if domain is None: raise ValueError(f"network_policy.transforms[{index}] missing required 'domain': {transform!r}") if not isinstance(domain, str): raise ValueError(f"network_policy.transforms[{index}].domain must be a string, got: {domain!r}") - transform["domain"] = _validate_domain_pattern(domain) + normalized_transform = dict(transform) + normalized_transform["domain"] = _validate_domain_pattern(domain) + normalized_transforms.append(normalized_transform) + policy["transforms"] = normalized_transforms🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@leap0/models/sandbox.py` around lines 86 - 95, The loop over transforms accepts any Mapping but then writes transform["domain"], which will raise for immutable mappings; instead normalize each transform to a mutable dict before mutating: in the for loop (the block that enumerates transforms) convert the item with transform = dict(transform) (or new = dict(transform); transforms[index] = new) and then call transform["domain"] = _validate_domain_pattern(domain) so you only mutate a mutable dict; alternatively enforce MutableMapping in the signature if you prefer, but the safe fix is to wrap each transform with dict(...) and update transforms[index] accordingly.
🤖 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/models/sandbox.py`:
- Around line 37-40: The validator _validate_domain_pattern assumes value is a
str and calls value.strip(), which can raise AttributeError for non-strings; add
a runtime type guard at the start of _validate_domain_pattern (e.g., check
isinstance(value, str)) and if the check fails raise ValueError("network policy
domains must be non-empty") or a clearer ValueError indicating invalid type,
then proceed to call strip() and the existing empty check so malformed inputs
(e.g., integers in allow_domains) produce the expected ValueError.
- Around line 86-95: The loop over transforms accepts any Mapping but then
writes transform["domain"], which will raise for immutable mappings; instead
normalize each transform to a mutable dict before mutating: in the for loop (the
block that enumerates transforms) convert the item with transform =
dict(transform) (or new = dict(transform); transforms[index] = new) and then
call transform["domain"] = _validate_domain_pattern(domain) so you only mutate a
mutable dict; alternatively enforce MutableMapping in the signature if you
prefer, but the safe fix is to wrap each transform with dict(...) and update
transforms[index] accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 66244673-b158-4175-95ac-dae8f717252c
📒 Files selected for processing (8)
leap0/_async/filesystem.pyleap0/_sync/desktop.pyleap0/_sync/filesystem.pyleap0/_utils/multipart.pyleap0/models/sandbox.pytests/_async/test_filesystem.pytests/_sync/test_filesystem.pytests/models/test_sandbox.py
✅ Files skipped from review due to trivial changes (2)
- tests/_async/test_filesystem.py
- tests/_sync/test_filesystem.py
🚧 Files skipped from review as they are similar to previous changes (3)
- leap0/_utils/multipart.py
- tests/models/test_sandbox.py
- leap0/_sync/desktop.py
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
Chores