Add presigned URL APIs#15
Merged
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 2 minutes and 4 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 (1)
📝 WalkthroughWalkthroughAdds presigned URL support for sandbox ports (models, schemas, sync/async client methods, validation, error interception), updates SSH client methods to target per-credential IDs, updates README and examples, and adds tests for presigned URLs and SSH ID-based operations. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Sandbox as "Sandbox (helper)"
participant SandboxesClient as "SandboxesClient"
participant API as "API Server"
Client->>Sandbox: create_presigned_url(port, expires_in)
Sandbox->>SandboxesClient: create_presigned_url(sandbox_ref, port, expires_in)
SandboxesClient->>SandboxesClient: CreatePresignedURLParams -> to_payload()
SandboxesClient->>API: POST /v1/sandbox/{id}/presigned-url {port, expires_in}
API-->>SandboxesClient: 201 Created {id, token, url, port, expires_at, ...}
SandboxesClient->>SandboxesClient: PresignedURL.from_dict(response)
SandboxesClient-->>Sandbox: PresignedURL
Sandbox-->>Client: PresignedURL
Client->>Sandbox: delete_presigned_url(presigned_id)
Sandbox->>SandboxesClient: delete_presigned_url(sandbox_ref, presigned_id)
SandboxesClient->>SandboxesClient: validate presigned_id
SandboxesClient->>API: DELETE /v1/sandbox/{id}/presigned-url/{presigned_id}
API-->>SandboxesClient: 204 No Content
SandboxesClient-->>Sandbox: None
Sandbox-->>Client: None
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 2
🧹 Nitpick comments (1)
tests/_sync/test_sandboxes.py (1)
112-115: Assert the 201 response contract in this create test.Line 112 only checks method/path/body today. If
expected_status=201gets dropped from the client call, this test still passes and the regression slips through. Please assert it here and mirror the same assertion in the async twin.Suggested test assertion
args, kwargs = mock_transport.request_json.call_args assert args == ("POST", "/v1/sandbox/sbx-1/presigned-url") assert kwargs["json"] == {"port": 8080, "expires_in": 900} + assert kwargs["expected_status"] == 201 assert result.token == "tok_1"🤖 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 112 - 115, The test currently asserts method/path/body and result.token but does not assert the expected_status contract, so update the assertion on mock_transport.request_json.call_args to also verify kwargs["expected_status"] == 201 (i.e., ensure the client call included expected_status=201); then apply the same addition to the async counterpart test (the async version that also calls mock_transport.request_json) so both sync and async tests assert expected_status explicitly.
🤖 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/models/sandbox.py`:
- Around line 291-310: The from_dict method currently defaults missing
expires_at/created_at to empty strings which masks malformed responses; update
PresignedURL.from_dict to validate that data["expires_at"] and
data["created_at"] are present and are non-empty strings (same style as the
earlier checks for id/token/url/host/sandbox_id) and raise a ValueError if they
are missing or invalid before calling cls(...) so the model construction fails
fast on bad API responses.
- Around line 277-288: The default dataclass-generated __repr__ for PresignedURL
leaks sensitive fields (token and url); implement a custom __repr__ (or __str__)
on the PresignedURL dataclass that returns a string containing non-sensitive
fields (e.g., id, host, sandbox_id, port, expires_at, created_at) while
replacing token and url with a fixed redaction marker like "<redacted>" so any
logging or debugging won't expose credentials; keep the class as
`@dataclass`(slots=True) and ensure the method name is __repr__ in the
PresignedURL class to override the default.
---
Nitpick comments:
In `@tests/_sync/test_sandboxes.py`:
- Around line 112-115: The test currently asserts method/path/body and
result.token but does not assert the expected_status contract, so update the
assertion on mock_transport.request_json.call_args to also verify
kwargs["expected_status"] == 201 (i.e., ensure the client call included
expected_status=201); then apply the same addition to the async counterpart test
(the async version that also calls mock_transport.request_json) so both sync and
async tests assert expected_status explicitly.
🪄 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: ea7c2b08-883a-41f7-8761-82453becdf4e
📒 Files selected for processing (9)
README.mdleap0/__init__.pyleap0/_async/sandbox.pyleap0/_schemas/sandbox.pyleap0/_sync/sandbox.pyleap0/models/sandbox.pytests/_async/test_sandboxes.pytests/_sync/test_sandboxes.pytests/models/test_sandbox.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/models/sandbox.py`:
- Around line 310-316: The port validation in the PresignedURL response parsing
only checks type; update the logic in the from_dict (or the function/method
containing the shown code) to enforce a TCP port range by verifying that port is
an int and 1 <= port <= 65535, and raise a ValueError that includes the invalid
port value (similar style to the existing error messages) when the check fails;
leave the existing timestamp checks for expires_at/created_at unchanged.
🪄 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: dc8657ed-a717-47b0-8fae-2fdde93bac09
📒 Files selected for processing (5)
leap0/_schemas/sandbox.pyleap0/models/sandbox.pytests/_async/test_sandboxes.pytests/_sync/test_sandboxes.pytests/models/test_sandbox.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/models/test_sandbox.py
- leap0/_schemas/sandbox.py
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
leap0/_sync/ssh.py (1)
12-22:⚠️ Potential issue | 🟡 MinorUse
ssh_commandin the docstring example.
SshAccessexposesssh_command, notcommand, so this example currently raisesAttributeError. The same typo is mirrored inleap0/_async/ssh.py.📝 Proposed fix
- print(access.command) + print(access.ssh_command)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@leap0/_sync/ssh.py` around lines 12 - 22, The docstring example references a non-existent attribute `command` and should use `ssh_command` exposed by the SshAccess object; update the example in the SshAccess docstrings (both synchronous and asynchronous modules where present) to call `ssh_command` (e.g., replace `access.command` with `access.ssh_command`) so the example does not raise AttributeError and reflects the actual API surface of SshAccess.
🧹 Nitpick comments (1)
tests/_sync/test_ssh.py (1)
16-19: Add coverage for the new validate/regenerate request shapes.This only exercises
delete_access, but the riskier SSH changes are invalidate_accessandregenerate_access: both URL templates changed, andvalidate_accessalso changed its JSON body. Please add one sync and one async assertion for/ssh/{id}/validateand/ssh/{id}/regenso those paths cannot regress unnoticed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/_sync/test_ssh.py` around lines 16 - 19, Add assertions covering the new validate and regenerate request shapes: in tests/_sync/test_ssh.py extend the test (or add a new one) to call SshClient(mock_transport).validate_access("sbx-1", id="ssh-1", body=...) and SshClient(mock_transport).regenerate_access("sbx-1", id="ssh-1") and assert mock_transport.request was called with the correct method+path ("POST", "/v1/sandbox/sbx-1/ssh/ssh-1/validate") and ("POST", "/v1/sandbox/sbx-1/ssh/ssh-1/regen"); for validate_access also assert the JSON body in mock_transport.request.call_args (e.g., call_args[1]['json']) matches the new expected payload. Mirror those checks in the async test file (tests/_async/test_ssh.py) by awaiting AsyncSshClient(...).validate_access and .regenerate_access and asserting the same mock_transport.request call_args for method, path and validate JSON body so both sync and async paths are covered.
🤖 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/_sync/ssh.py`:
- Line 49: Rename the shadowing parameter named id to a more explicit name
(e.g., access_id) in the SSH client methods to satisfy Ruff A002 and match the
project naming pattern; update the parameter name and its type hint in the
method signatures (e.g., delete_access, get_access, and the other access-related
methods in leap0/_sync/ssh.py and the matching ones in leap0/_async/ssh.py),
update all uses inside each method body, docstrings, and any callers/tests to
use access_id, and ensure both sync and async versions keep identical parameter
names for API symmetry.
In `@tests/models/test_sandbox.py`:
- Line 115: The test's pytest.raises invocation uses a normal string for the
regex match which triggers Ruff RUF043; update the match pattern in the with
pytest.raises(ValueError, match="invalid 'port'.*0") call to use a raw string
literal (e.g., r"invalid 'port'.*0") so the backslashes/regex metacharacters are
not treated as escape sequences and Ruff stops warning; locate the assertion in
tests/models/test_sandbox.py within the test function containing the with
pytest.raises(...) block and replace the match argument with a raw string.
- Around line 79-84: The test test_rejects_invalid_values expects ValueError but
Pydantic v2 wraps errors raised in `@model_validator`(mode="after") into
pydantic.ValidationError; update the test to assert
pytest.raises(ValidationError, ...) for both CreatePresignedURLParams
constructions (and add/import pydantic.ValidationError) so the invalid port and
expires_in cases correctly expect the Pydantic ValidationError.
---
Outside diff comments:
In `@leap0/_sync/ssh.py`:
- Around line 12-22: The docstring example references a non-existent attribute
`command` and should use `ssh_command` exposed by the SshAccess object; update
the example in the SshAccess docstrings (both synchronous and asynchronous
modules where present) to call `ssh_command` (e.g., replace `access.command`
with `access.ssh_command`) so the example does not raise AttributeError and
reflects the actual API surface of SshAccess.
---
Nitpick comments:
In `@tests/_sync/test_ssh.py`:
- Around line 16-19: Add assertions covering the new validate and regenerate
request shapes: in tests/_sync/test_ssh.py extend the test (or add a new one) to
call SshClient(mock_transport).validate_access("sbx-1", id="ssh-1", body=...)
and SshClient(mock_transport).regenerate_access("sbx-1", id="ssh-1") and assert
mock_transport.request was called with the correct method+path ("POST",
"/v1/sandbox/sbx-1/ssh/ssh-1/validate") and ("POST",
"/v1/sandbox/sbx-1/ssh/ssh-1/regen"); for validate_access also assert the JSON
body in mock_transport.request.call_args (e.g., call_args[1]['json']) matches
the new expected payload. Mirror those checks in the async test file
(tests/_async/test_ssh.py) by awaiting AsyncSshClient(...).validate_access and
.regenerate_access and asserting the same mock_transport.request call_args for
method, path and validate JSON body so both sync and async paths are covered.
🪄 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: f8d55f0e-0b7e-4b2b-ab6e-ce66509b90e1
📒 Files selected for processing (7)
examples/ssh.pyleap0/_async/ssh.pyleap0/_sync/ssh.pyleap0/models/sandbox.pytests/_async/test_ssh.pytests/_sync/test_ssh.pytests/models/test_sandbox.py
🚧 Files skipped from review as they are similar to previous changes (1)
- leap0/models/sandbox.py
| @intercept_errors("Failed to delete SSH access: ") | ||
| def delete_access(self, sandbox: SandboxRef, http_timeout: float | None = None) -> None: | ||
| """Revoke SSH access for a sandbox. The credential is invalidated immediately. | ||
| def delete_access(self, sandbox: SandboxRef, *, id: str, http_timeout: float | None = None) -> None: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Ruff config =="
fd -HI '^(pyproject\.toml|ruff\.toml|\.ruff\.toml|setup\.cfg)$' -x sh -c '
file="$1"
echo "--- $file ---"
rg -n -C2 "A002|per-file-ignores|ignore|select" "$file" || true
' sh {}
echo
echo "== SSH methods using `id` as a parameter =="
rg -nP --type=py '\b(?:async\s+def|def)\s+(delete_access|validate_access|regenerate_access)\([^)]*\bid\s*:' leap0Repository: leap0-dev/leap0-python
Length of output: 1512
🏁 Script executed:
#!/bin/bash
# Check for API documentation or interface definitions
echo "== Checking for interface/protocol definitions =="
rg -n "class SshAccess|class.*Access|protocol.*Access" leap0 -A 5 | head -50
echo
echo "== Checking similar methods in related classes =="
rg -n "def (delete|validate|regenerate)_" leap0 -B 2 -A 2 --type=py | head -80
echo
echo "== Looking for any inline suppression comments or docstrings =="
grep -n "noqa\|type: ignore\|A002" leap0/_sync/ssh.py leap0/_async/ssh.py || echo "No inline suppressions found"Repository: leap0-dev/leap0-python
Length of output: 6284
Rename id parameter to a more explicit name like access_id in SSH client methods.
Three methods in both sync and async SSH clients expose id as a parameter, which Ruff flags as A002 (builtin shadowing). This contradicts the codebase pattern where similar resource methods use explicit names (presigned_url_id, recording_id). Rename to access_id or credential_id, or add a targeted suppression if API symmetry is required.
Applies to:
leap0/_sync/ssh.py: lines 49, 60, 81leap0/_async/ssh.py: lines 49, 60, 81
🧰 Tools
🪛 Ruff (0.15.9)
[error] 49-49: Function argument id is shadowing a Python builtin
(A002)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@leap0/_sync/ssh.py` at line 49, Rename the shadowing parameter named id to a
more explicit name (e.g., access_id) in the SSH client methods to satisfy Ruff
A002 and match the project naming pattern; update the parameter name and its
type hint in the method signatures (e.g., delete_access, get_access, and the
other access-related methods in leap0/_sync/ssh.py and the matching ones in
leap0/_async/ssh.py), update all uses inside each method body, docstrings, and
any callers/tests to use access_id, and ensure both sync and async versions keep
identical parameter names for API symmetry.
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
Summary by CodeRabbit
New Features
Documentation
Tests