update snapshot restore and sandbox snapshot APIs#18
Merged
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 27 minutes and 39 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThe PR refactors the snapshot API by moving snapshot creation from a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Author
|
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
leap0/_async/snapshots.py (1)
85-100:⚠️ Potential issue | 🟡 MinorDocstring has a duplicated
Args:section (mirrors sync client).Same issue as
leap0/_sync/snapshots.py: twoArgs:blocks withhttp_timeoutlisted twice. Please consolidate.📝 Proposed fix
"""Restore a sandbox from a snapshot. Args: snapshot_name: Name of the snapshot to restore. auto_pause: Automatically pause the restored sandbox on timeout. timeout: Sandbox timeout in seconds. network_policy: Override the network policy from the snapshot. - - http_timeout: Optional HTTP request timeout in seconds for this SDK call. - - Args: http_timeout: Optional HTTP request timeout in seconds for this SDK call. Returns: Sandbox: Newly restored sandbox. """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@leap0/_async/snapshots.py` around lines 85 - 100, The docstring for the async restore function in leap0/_async/snapshots.py contains a duplicated "Args:" section (and duplicates the http_timeout entry); consolidate into a single "Args:" block listing snapshot_name, auto_pause, timeout, network_policy, and http_timeout only once, matching the sync version (leap0/_sync/snapshots.py) and update the Returns section as-is; target the docstring for the restore function in this module.leap0/_sync/snapshots.py (1)
85-100:⚠️ Potential issue | 🟡 MinorDocstring has a duplicated
Args:section.Lines 87–96 contain two
Args:headers, withhttp_timeoutalso documented twice. This breaks rendering by most docstring parsers (Sphinx/Napoleon, MkDocs, etc.). Consolidate into one section.📝 Proposed fix
"""Restore a sandbox from a snapshot. Args: snapshot_name: Name of the snapshot to restore. auto_pause: Automatically pause the restored sandbox on timeout. timeout: Sandbox timeout in seconds. network_policy: Override the network policy from the snapshot. - - http_timeout: Optional HTTP request timeout in seconds for this SDK call. - - Args: http_timeout: Optional HTTP request timeout in seconds for this SDK call. Returns: Sandbox: Newly restored sandbox. """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@leap0/_sync/snapshots.py` around lines 85 - 100, The docstring for the "Restore a sandbox from a snapshot." function in leap0/_sync/snapshots.py contains two duplicated "Args:" sections (including http_timeout documented twice); consolidate them into a single "Args:" block listing snapshot_name, auto_pause, timeout, network_policy, and http_timeout once, remove the duplicate header/entry, and ensure the "Returns:" section remains unchanged so docstring parsers render correctly.
🧹 Nitpick comments (6)
tests/_async/test_sandboxes.py (1)
346-357: Also assert the delegated snapshot result is returned.This helper test currently passes even if
AsyncSandbox.create_snapshot()calls the client but forgets to return its result.Suggested test tightening
) calls: list[tuple[object, str | None, bool, float | None]] = [] + expected_snapshot = object() async def create_snapshot(sandbox: object, *, name: str | None = None, kill_sandbox_after: bool = False, http_timeout: float | None = None): calls.append((sandbox, name, kill_sandbox_after, http_timeout)) - return None + return expected_snapshot sandboxes.create_snapshot = create_snapshot sandbox = AsyncSandbox(fake_client, Sandbox(id="sbx-1", state="running")) - await sandbox.create_snapshot(name="snap-a", kill_sandbox_after=True, http_timeout=1.5) + result = await sandbox.create_snapshot(name="snap-a", kill_sandbox_after=True, http_timeout=1.5) assert calls == [(sandbox, "snap-a", True, 1.5)] + assert result is expected_snapshot🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/_async/test_sandboxes.py` around lines 346 - 357, Test only verifies the client was called but not that AsyncSandbox.create_snapshot returns the delegated result; modify the local async create_snapshot helper (assigned to sandboxes.create_snapshot) to return a distinct sentinel value (e.g., object() or a string) and then assert that await sandbox.create_snapshot(...) equals that sentinel so the test fails if AsyncSandbox.create_snapshot forgets to return the client's result; reference the create_snapshot helper and AsyncSandbox.create_snapshot in the change.tests/_sync/test_sandboxes.py (1)
422-444: Assertcreate_snapshot()returns the delegated result.The test would still pass if
RichSandbox.create_snapshot()invokedsandboxes.create_snapshot()but returnedNone.Suggested test tightening
) sandbox = RichSandbox(client, Sandbox(id="sbx-1", state="running")) + expected_snapshot = MagicMock(id="snap-1") + sandboxes.create_snapshot.return_value = expected_snapshot - sandbox.create_snapshot(name="snap-a", kill_sandbox_after=True, http_timeout=1.5) + result = sandbox.create_snapshot(name="snap-a", kill_sandbox_after=True, http_timeout=1.5) sandboxes.create_snapshot.assert_called_once_with( sandbox, name="snap-a", kill_sandbox_after=True, http_timeout=1.5, ) + assert result is expected_snapshot🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/_sync/test_sandboxes.py` around lines 422 - 444, The test test_create_snapshot_delegates_to_sandboxes_client should also assert that RichSandbox.create_snapshot returns whatever sandboxes.create_snapshot returns; update the test to set a sentinel return value on the sandboxes MagicMock (sandboxes.create_snapshot) and then assert the call's return equals that sentinel when calling RichSandbox.create_snapshot, referencing RichSandbox.create_snapshot and sandboxes.create_snapshot to locate where to set the return_value and where to add the equality assertion.leap0/_async/sandbox.py (1)
132-154: Add the public return type annotation.
AsyncSandboxesClient.create_snapshot()is typed as returningSnapshot; the boundAsyncSandbox.create_snapshot()helper should expose the same type to SDK users.Suggested typing polish
async def create_snapshot( self, *, name: str | None = None, kill_sandbox_after: bool = False, http_timeout: float | None = None, - ): + ) -> Snapshot:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@leap0/_async/sandbox.py` around lines 132 - 154, The create_snapshot method on AsyncSandbox should declare and expose the public return type Snapshot to match AsyncSandboxesClient.create_snapshot; update the AsyncSandbox.create_snapshot signature to include the return annotation -> Snapshot and ensure Snapshot is imported or referenced from its defining module so the helper's type matches the client method.tests/_sync/test_snapshots.py (1)
49-57: Add a happy-path restore transport test.The current coverage catches invalid input and payload trimming, but a wrong restore method/path/body or response parsing regression could still slip through.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/_sync/test_snapshots.py` around lines 49 - 57, Add a happy-path unit test that uses the existing mock_transport fixture to verify SnapshotsClient.restore sends the correct HTTP method/path/body and correctly parses the response: set mock_transport to return a representative JSON payload, call SnapshotsClient(mock_transport).restore(snapshot_name="snap-1"), assert mock_transport.request (or equivalent send call used in tests) was invoked with the expected method, endpoint and payload matching RestoreSnapshotParams.to_payload(), and assert the returned object matches the parsed response (e.g., fields on the RestoreSnapshotResponse or whatever type SnapshotsClient.restore returns).leap0/models/snapshot.py (1)
32-36: Redundant reassignment into_payload.
_validate_valuesalready strips and writes backself.snapshot_name, somodel_dump(exclude_none=True)will already emit the stripped value. The explicitpayload["snapshot_name"] = self.snapshot_nameline is a no-op.♻️ Proposed cleanup
def to_payload(self) -> dict[str, object]: """Convert this object to an API request payload.""" - payload = self.model_dump(exclude_none=True) - payload["snapshot_name"] = self.snapshot_name - return payload + return self.model_dump(exclude_none=True)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@leap0/models/snapshot.py` around lines 32 - 36, The to_payload method in Snapshot currently reassigns payload["snapshot_name"] = self.snapshot_name after calling model_dump(exclude_none=True), which is redundant because _validate_values already normalizes snapshot_name and model_dump will include the stripped value; remove the explicit reassignment line from Snapshot.to_payload so the method simply returns the model_dump payload (keeping model_dump(exclude_none=True) and the docstring intact) to avoid the no-op.leap0/_sync/sandbox.py (1)
129-151: Add-> Snapshotreturn annotation for consistency.
SandboxesClient.create_snapshotis annotated-> Snapshot, but the handle method here has no return annotation, which diverges from sibling handle methods (pause,create_presigned_url, etc.) and from the docstring's declared return type.♻️ Proposed fix
def create_snapshot( self, *, name: str | None = None, kill_sandbox_after: bool = False, http_timeout: float | None = None, - ): + ) -> Snapshot:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@leap0/_sync/sandbox.py` around lines 129 - 151, The create_snapshot method on the sandbox handle lacks a return type annotation; update its signature to include "-> Snapshot" to match SandboxesClient.create_snapshot and sibling handlers (e.g., pause, create_presigned_url) and the docstring; ensure the Snapshot type is imported or referenced appropriately so the annotation resolves.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@leap0/_async/snapshots.py`:
- Around line 85-100: The docstring for the async restore function in
leap0/_async/snapshots.py contains a duplicated "Args:" section (and duplicates
the http_timeout entry); consolidate into a single "Args:" block listing
snapshot_name, auto_pause, timeout, network_policy, and http_timeout only once,
matching the sync version (leap0/_sync/snapshots.py) and update the Returns
section as-is; target the docstring for the restore function in this module.
In `@leap0/_sync/snapshots.py`:
- Around line 85-100: The docstring for the "Restore a sandbox from a snapshot."
function in leap0/_sync/snapshots.py contains two duplicated "Args:" sections
(including http_timeout documented twice); consolidate them into a single
"Args:" block listing snapshot_name, auto_pause, timeout, network_policy, and
http_timeout once, remove the duplicate header/entry, and ensure the "Returns:"
section remains unchanged so docstring parsers render correctly.
---
Nitpick comments:
In `@leap0/_async/sandbox.py`:
- Around line 132-154: The create_snapshot method on AsyncSandbox should declare
and expose the public return type Snapshot to match
AsyncSandboxesClient.create_snapshot; update the AsyncSandbox.create_snapshot
signature to include the return annotation -> Snapshot and ensure Snapshot is
imported or referenced from its defining module so the helper's type matches the
client method.
In `@leap0/_sync/sandbox.py`:
- Around line 129-151: The create_snapshot method on the sandbox handle lacks a
return type annotation; update its signature to include "-> Snapshot" to match
SandboxesClient.create_snapshot and sibling handlers (e.g., pause,
create_presigned_url) and the docstring; ensure the Snapshot type is imported or
referenced appropriately so the annotation resolves.
In `@leap0/models/snapshot.py`:
- Around line 32-36: The to_payload method in Snapshot currently reassigns
payload["snapshot_name"] = self.snapshot_name after calling
model_dump(exclude_none=True), which is redundant because _validate_values
already normalizes snapshot_name and model_dump will include the stripped value;
remove the explicit reassignment line from Snapshot.to_payload so the method
simply returns the model_dump payload (keeping model_dump(exclude_none=True) and
the docstring intact) to avoid the no-op.
In `@tests/_async/test_sandboxes.py`:
- Around line 346-357: Test only verifies the client was called but not that
AsyncSandbox.create_snapshot returns the delegated result; modify the local
async create_snapshot helper (assigned to sandboxes.create_snapshot) to return a
distinct sentinel value (e.g., object() or a string) and then assert that await
sandbox.create_snapshot(...) equals that sentinel so the test fails if
AsyncSandbox.create_snapshot forgets to return the client's result; reference
the create_snapshot helper and AsyncSandbox.create_snapshot in the change.
In `@tests/_sync/test_sandboxes.py`:
- Around line 422-444: The test
test_create_snapshot_delegates_to_sandboxes_client should also assert that
RichSandbox.create_snapshot returns whatever sandboxes.create_snapshot returns;
update the test to set a sentinel return value on the sandboxes MagicMock
(sandboxes.create_snapshot) and then assert the call's return equals that
sentinel when calling RichSandbox.create_snapshot, referencing
RichSandbox.create_snapshot and sandboxes.create_snapshot to locate where to set
the return_value and where to add the equality assertion.
In `@tests/_sync/test_snapshots.py`:
- Around line 49-57: Add a happy-path unit test that uses the existing
mock_transport fixture to verify SnapshotsClient.restore sends the correct HTTP
method/path/body and correctly parses the response: set mock_transport to return
a representative JSON payload, call
SnapshotsClient(mock_transport).restore(snapshot_name="snap-1"), assert
mock_transport.request (or equivalent send call used in tests) was invoked with
the expected method, endpoint and payload matching
RestoreSnapshotParams.to_payload(), and assert the returned object matches the
parsed response (e.g., fields on the RestoreSnapshotResponse or whatever type
SnapshotsClient.restore returns).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1c14cd49-12e3-40c1-85d1-cd72ef791c46
📒 Files selected for processing (13)
examples/snapshots.pyleap0/__init__.pyleap0/_async/sandbox.pyleap0/_async/snapshots.pyleap0/_sync/sandbox.pyleap0/_sync/snapshots.pyleap0/models/sandbox.pyleap0/models/snapshot.pytests/_async/test_sandboxes.pytests/_async/test_snapshots.pytests/_sync/test_sandboxes.pytests/_sync/test_snapshots.pytests/test_import.py
💤 Files with no reviewable changes (1)
- tests/_async/test_snapshots.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
create_snapshot()method.Refactor
resume()torestore().