Refactor SDK resource API - #25
Conversation
…nce disk size handling - Introduced a new `Computer` class for easier management of computer instances. - Updated methods for creating and managing computers to accept user-friendly disk size formats (e.g., "2gb"). - Enhanced error handling for conflicting disk size parameters. - Added methods for publishing and managing ports on computers. - Updated README documentation to reflect new usage patterns and examples. - Added tests to ensure functionality of new features and refactored existing tests for clarity.
|
Warning Review limit reached
Next review available in: 41 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe Changesjs/ to ts/ directory rename
Python SDK: client rename and Computer wrapper
TypeScript SDK: Computer wrapper, published ports, disk sizing
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant Computer
participant ComputersClient
participant Backend
Caller->>Computer: Computer.create(params)
Computer->>ComputersClient: create(body with resolved disk_size_mb)
ComputersClient->>Backend: POST /computers
Backend-->>ComputersClient: ComputerInfoWire
ComputersClient-->>Computer: ComputerInfo
Computer-->>Caller: Computer instance
Caller->>Computer: computer.publishPort(port)
Computer->>ComputersClient: publishPort(computerId, port)
ComputersClient->>Backend: POST /computers/:id/published-ports
Backend-->>ComputersClient: published port payload
ComputersClient-->>Computer: ComputerPublishedPortInfo
Computer-->>Caller: public URL
Caller->>Computer: computer.delete()
Computer->>ComputersClient: delete(computerId)
ComputersClient->>Backend: DELETE /computers/:id
Backend-->>ComputersClient: confirmation
ComputersClient-->>Computer: updated info
Computer-->>Caller: this
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/celesto/sdk/computer.py (1)
147-313: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocstrings are light on Args/Returns.
Public methods like
get,list,run,run_stream,publish_port,unpublish_port, etc. carry a single summary line rather than full Google-styleArgs/Returnssections, unlike the fuller docstrings inclient.py'sComputersclass. As per coding guidelines, "Write Google-style docstrings for all public methods."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/celesto/sdk/computer.py` around lines 147 - 313, Public methods in Computer are missing the required Google-style docstrings, with only one-line summaries instead of Args/Returns sections. Update the docstrings for the public API in Computer, including get, list, list_templates, templates, data, refresh, run, exec, run_stream, stop, start, delete, publish_port, list_published_ports, and unpublish_port, using the same Google-style structure used in Computers from client.py. Make sure each method documents its parameters, return value, and any relevant behavior consistently so the class matches the project’s docstring standard.Source: Coding guidelines
README.md (1)
354-374: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win"curie" appears out of thin air.
Every other example in this file either creates its own
Computer()or explains wherecomputer_idcomes from (Line 332-333: "computer_idcan becomputer.idfromComputer()or a computer name shown bycelesto computer list"). The Publish Ports snippets jump straight toComputer.get("curie")with no setup — a reader has no idea where"curie"came from or how to get their own name to substitute. As per coding guidelines, "Introduce required keys, IDs, names, or variables before using them in commands or code samples in documentation" and "Show how generated resource names are created or referencecelesto computer create/celesto computer listwhen documenting CLI generated names."✍️ Proposed fix
### Publish Ports Publish a port when a service inside the computer needs a public URL: ```python from celesto import Computer -computer = Computer.get("curie") +# computer_id is Computer().id, or a name from `celesto computer list` +computer = Computer.get(computer_id) url = computer.publish_port(8000) print(url)Apply the same substitution to the second snippet. </details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@README.mdaround lines 354 - 374, The Publish Ports examples use a hardcoded
Computer.get("curie") without introducing where that identifier comes from,
which makes the snippet unclear. Update both snippets in the Publish Ports
section to use a descriptive placeholder such as computer_id and add a brief
note near the Computer.get call explaining that the value should come from
Computer().id or a name from celesto computer list. Keep the examples aligned
with the earlier Computer and computer_id guidance already used elsewhere in the
README.</details> <!-- cr-comment:v1:ec3e0c6cc0b13ab6d0200f9e --> _Source: Coding guidelines_ </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.Inline comments:
In@src/celesto/__init__.py:
- Around line 4-8:
__all__in the module-level__init__is out of Ruff/isort
order and triggers RUF022. Update the__all__declaration in
src/celesto/__init__.pyso the exported names are sorted in the expected
order, keepingComputergrouped before the lowercase entry and preserving
__version__as needed. Use the existing__all__symbol in__init__.pyas
the single place to fix the ordering.In
@src/celesto/sdk/computer.py:
- Around line 147-165: The
Computer.getclassmethod is shadowing the
MutableMapping.getinstance method, so calls likecomputer.get("name", default)are being routed to the API loader instead of the mapping lookup.
Rename the classmethod inComputerto a distinct loader name (and update any
internal call sites or references) so the mapping-style.get()remains
available on instances and behaves like a normal dictionary helper.In
@tests/test_sdk.py:
- Around line 41-45: Add test coverage for the
Computer.get()fetch-by-id path
intests/test_sdk.pyby creating a test that callsComputer.get(...)on the
computersclient and verifies it returns the expected object, so the
MutableMapping.get()collision is exercised. Then add a separate assertion for
the dictionary-style.get(key, default)behavior on the same
Computer/computerssymbol after the rename, ensuring both method meanings
are covered independently.
Nitpick comments:
In@README.md:
- Around line 354-374: The Publish Ports examples use a hardcoded
Computer.get("curie") without introducing where that identifier comes from,
which makes the snippet unclear. Update both snippets in the Publish Ports
section to use a descriptive placeholder such as computer_id and add a brief
note near the Computer.get call explaining that the value should come from
Computer().id or a name from celesto computer list. Keep the examples aligned
with the earlier Computer and computer_id guidance already used elsewhere in the
README.In
@src/celesto/sdk/computer.py:
- Around line 147-313: Public methods in Computer are missing the required
Google-style docstrings, with only one-line summaries instead of Args/Returns
sections. Update the docstrings for the public API in Computer, including get,
list, list_templates, templates, data, refresh, run, exec, run_stream, stop,
start, delete, publish_port, list_published_ports, and unpublish_port, using the
same Google-style structure used in Computers from client.py. Make sure each
method documents its parameters, return value, and any relevant behavior
consistently so the class matches the project’s docstring standard.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro **Run ID**: `9d240080-118a-447d-b75f-8cc0e711ed3c` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between cb8d9ba91b2a402eaba1d8b65748e8f089c2061c and 512c0d086cf88335f16b2e182c2024e0860bb609. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `ts/package-lock.json` is excluded by `!**/package-lock.json` </details> <details> <summary>📒 Files selected for processing (33)</summary> * `.github/workflows/release.yml` * `.github/workflows/ts-test.yml` * `.gitignore` * `CLAUDE.md` * `README.md` * `src/celesto/__init__.py` * `src/celesto/auth.py` * `src/celesto/computer.py` * `src/celesto/deployment.py` * `src/celesto/integrations/openai_agents/hosted.py` * `src/celesto/sdk/__init__.py` * `src/celesto/sdk/client.py` * `src/celesto/sdk/computer.py` * `src/celesto/sdk/exceptions.py` * `tests/test_sdk.py` * `ts/LICENSE` * `ts/README.md` * `ts/package.json` * `ts/src/computers/client.ts` * `ts/src/computers/computer.ts` * `ts/src/computers/index.ts` * `ts/src/computers/types.ts` * `ts/src/core/config.ts` * `ts/src/core/errors.ts` * `ts/src/core/http.ts` * `ts/src/gatekeeper/client.ts` * `ts/src/gatekeeper/index.ts` * `ts/src/gatekeeper/types.ts` * `ts/src/index.ts` * `ts/test.mjs` * `ts/tests/computers.test.ts` * `ts/tsconfig.json` * `ts/tsup.config.ts` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| from .sdk import Computer | ||
|
|
||
| __version__ = "0.0.9" | ||
|
|
||
| __all__ = ["app", "Celesto", "__version__"] | ||
| __all__ = ["app", "Computer", "__version__"] |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Sort __all__, or the lint gate stops the whole operation.
Ruff's RUF022 flags Line 8 — __all__ isn't in isort order (CamelCase before the rest, natural sort within groups). A small thing, but a man who don't tidy his own house gets no respect on the street.
As per coding guidelines, "Use Ruff for linting and formatting Python code."
🧹 Proposed fix
-__all__ = ["app", "Computer", "__version__"]
+__all__ = ["Computer", "__version__", "app"]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| from .sdk import Computer | |
| __version__ = "0.0.9" | |
| __all__ = ["app", "Celesto", "__version__"] | |
| __all__ = ["app", "Computer", "__version__"] | |
| from .sdk import Computer | |
| __version__ = "0.0.9" | |
| __all__ = ["Computer", "__version__", "app"] |
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 8-8: __all__ is not sorted
Apply an isort-style sorting to __all__
(RUF022)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/celesto/__init__.py` around lines 4 - 8, `__all__` in the module-level
`__init__` is out of Ruff/isort order and triggers RUF022. Update the `__all__`
declaration in `src/celesto/__init__.py` so the exported names are sorted in the
expected order, keeping `Computer` grouped before the lowercase entry and
preserving `__version__` as needed. Use the existing `__all__` symbol in
`__init__.py` as the single place to fix the ordering.
Sources: Coding guidelines, Linters/SAST tools
| @classmethod | ||
| def get( | ||
| cls, | ||
| computer_id: str, | ||
| *, | ||
| api_key: str | None = None, | ||
| base_url: str | None = None, | ||
| client: Any | None = None, | ||
| ) -> "Computer": | ||
| """Load an existing computer by name or ID.""" | ||
| instance = cls.__new__(cls) | ||
| instance._client, instance._owns_client = _make_client( | ||
| client=client, | ||
| api_key=api_key, | ||
| base_url=base_url, | ||
| ) | ||
| instance._data = instance._client.computers.get(computer_id) | ||
| return instance | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Computer.get() shoots the family's own .get() in the foot.
Now listen. This class inherits MutableMapping, promising the usual dict tricks — computer.get("name", default) should be one of 'em. But you've gone and named the classmethod get too. A classmethod on a class always wins the lookup over __getattr__, and its descriptor binds to the class, not the instance — so computer.get("name") doesn't touch self._data at all. It calls Computer.get(computer_id="name"), tries to fetch a computer named "name" from the API, and blows up with a TypeError the moment anyone passes a default argument like real dict-.get() users do. Nobody tested this path, and it'll surprise every user who treats this thing as the dict it claims to be.
Rename the classmethod so the Mapping contract stays intact.
🔧 Proposed fix — free up `.get()` for Mapping
- `@classmethod`
- def get(
+ `@classmethod`
+ def from_id(
cls,
computer_id: str,
*,
api_key: str | None = None,
base_url: str | None = None,
client: Any | None = None,
) -> "Computer":
"""Load an existing computer by name or ID."""📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @classmethod | |
| def get( | |
| cls, | |
| computer_id: str, | |
| *, | |
| api_key: str | None = None, | |
| base_url: str | None = None, | |
| client: Any | None = None, | |
| ) -> "Computer": | |
| """Load an existing computer by name or ID.""" | |
| instance = cls.__new__(cls) | |
| instance._client, instance._owns_client = _make_client( | |
| client=client, | |
| api_key=api_key, | |
| base_url=base_url, | |
| ) | |
| instance._data = instance._client.computers.get(computer_id) | |
| return instance | |
| `@classmethod` | |
| def from_id( | |
| cls, | |
| computer_id: str, | |
| *, | |
| api_key: str | None = None, | |
| base_url: str | None = None, | |
| client: Any | None = None, | |
| ) -> "Computer": | |
| """Load an existing computer by name or ID.""" | |
| instance = cls.__new__(cls) | |
| instance._client, instance._owns_client = _make_client( | |
| client=client, | |
| api_key=api_key, | |
| base_url=base_url, | |
| ) | |
| instance._data = instance._client.computers.get(computer_id) | |
| return instance |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/celesto/sdk/computer.py` around lines 147 - 165, The `Computer.get`
classmethod is shadowing the `MutableMapping.get` instance method, so calls like
`computer.get("name", default)` are being routed to the API loader instead of
the mapping lookup. Rename the classmethod in `Computer` to a distinct loader
name (and update any internal call sites or references) so the mapping-style
`.get()` remains available on instances and behaves like a normal dictionary
helper.
| def test_internal_client_still_supports_cli_service_operations(): | ||
| client = _CelestoClient("test-key", base_url="http://localhost:8500/v1") | ||
| assert hasattr(client, "deployment") | ||
| assert hasattr(client, "gatekeeper") | ||
| assert hasattr(client, "computers") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== files ==\n'
git ls-files | rg '^(src/celesto/sdk/computer\.py|tests/test_sdk\.py|tests/.*\.py)$'
printf '\n== search Computer.get usages ==\n'
rg -n "Computer\.get\(|\.get\(" src tests -g '*.py'
printf '\n== outline computer.py ==\n'
ast-grep outline src/celesto/sdk/computer.py --view expanded
printf '\n== outline tests/test_sdk.py ==\n'
ast-grep outline tests/test_sdk.py --view expandedRepository: CelestoAI/sdk
Length of output: 13288
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== relevant lines in src/celesto/sdk/computer.py ==\n'
nl -ba src/celesto/sdk/computer.py | sed -n '1,260p'
printf '\n== relevant lines in tests/test_sdk.py ==\n'
nl -ba tests/test_sdk.py | sed -n '1,260p'Repository: CelestoAI/sdk
Length of output: 244
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== src/celesto/sdk/computer.py ==\n'
nl -ba src/celesto/sdk/computer.py | sed -n '130,190p'
printf '\n== tests/test_sdk.py ==\n'
nl -ba tests/test_sdk.py | sed -n '1,140p'
printf '\n== exact search for Computer.get in tests ==\n'
rg -n "Computer\.get\(" tests src -g '*.py'Repository: CelestoAI/sdk
Length of output: 226
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
def show(path, start, end):
print(f"\n== {path} {start}-{end} ==")
lines = Path(path).read_text().splitlines()
for i in range(start, min(end, len(lines)) + 1):
print(f"{i:4}: {lines[i-1]}")
show("src/celesto/sdk/computer.py", 130, 190)
show("tests/test_sdk.py", 1, 120)
show("tests/test_sdk.py", 120, 260)
PYRepository: CelestoAI/sdk
Length of output: 12260
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from collections.abc import MutableMapping
class M(MutableMapping):
`@classmethod`
def get(cls, x, **kwargs):
print("classmethod", cls.__name__, x, kwargs)
return "class"
def __getitem__(self, key):
return {"name": "n"}[key]
def __setitem__(self, key, value): pass
def __delitem__(self, key): pass
def __iter__(self): return iter(["name"])
def __len__(self): return 1
m = M()
print("instance attr:", m.get)
print("call result:", m.get("name"))
PYRepository: CelestoAI/sdk
Length of output: 252
Add coverage for Computer.get()
tests/test_sdk.py:41-45 never exercises the Computer.get(...) fetch-by-id path, so the MutableMapping.get() collision can still slip through. Add one test for that path; after the rename, add a separate .get(key, default) case.
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 41-41: Configuring an LLM/agent client endpoint over http:// sends prompts and responses (and often API keys) in cleartext, exposing them to interception. Use https for the base_url.
Context: base_url="http://localhost:8500/v1"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(llm-client-insecure-http-python)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_sdk.py` around lines 41 - 45, Add test coverage for the
`Computer.get()` fetch-by-id path in `tests/test_sdk.py` by creating a test that
calls `Computer.get(...)` on the `computers` client and verifies it returns the
expected object, so the `MutableMapping.get()` collision is exercised. Then add
a separate assertion for the dictionary-style `.get(key, default)` behavior on
the same `Computer`/`computers` symbol after the rename, ensuring both method
meanings are covered independently.
2 participants
Summary
Celesto().computersto the directComputerresource APInew Celesto().computerstoComputerand move the package folder fromjs/tots/Validation
uv run ruff check .uv run pytestcd ts && npm testcd ts && npm run lintcd ts && npm run buildSummary by CodeRabbit
New Features
Computerexperience for creating, loading, listing, running, and managing computers.Bug Fixes
Documentation
Computer-first usage and current TypeScript package location.