fix(sdk): safely handle cases where volume.host is unset - #1659
Conversation
github-actions
Bot
added
sdk/python
size/XS
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31a4705a96
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if not isinstance(volume.host, Unset): | ||
| api_host = ApiHost(path=volume.host.path) |
There was a problem hiding this comment.
When a valid PVC or OSSFS volume is converted, the domain model sets volume.host to None; because None is not an instance of the generated Unset class, this branch executes and accessing .path raises AttributeError. Host volumes similarly fail at the repeated PVC or OSSFS guards, so every sandbox creation containing a volume now fails before sending its request. Retain the None checks (or explicitly accept both representations) and add a request-mapping regression test.
AGENTS.md reference: sdks/AGENTS.md:L119-L120
Useful? React with 👍 / 👎.
31a4705 to
16ee117
Compare
github-actions
Bot
added
size/S
16ee117 to
7508844
Compare
Pangjiping
left a comment
Pangjiping
left a comment
There was a problem hiding this comment.
Blocking issues found, verified locally by parsing the changed file and running ruff:
- Syntax error at the
sub_pathguard (ifmisspelled asf) — the module cannot even be imported (SyntaxError: invalid syntax). - No regression test for #1658, and no local verification was run (ruff/pytest would have caught the syntax error).
- Root-cause concern: for the documented domain
Volumemodel the fields default toNone, neverUnset, so the new guards only fire for the generated APIVolumetype — please clarify the repro and cover it with a test.
Please also run uv run ruff check and uv run pytest on the Python SDK.
|
|
||
| api_sub_path = UNSET | ||
| if volume.sub_path is not None: | ||
| f volume.sub_path is not None and not isinstance(volume.sub_path, Unset): |
There was a problem hiding this comment.
Syntax error: if is misspelled as f. The module cannot be parsed at all (SyntaxError: invalid syntax), so the Python SDK is unimportable — verified locally with ast.parse and uv run ruff check. Please fix and run uv run ruff check and uv run pytest (the PR currently only ran the auto-label CI check).
|
|
||
| api_host = UNSET | ||
| if volume.host is not None: | ||
| if volume.host is not None and not isinstance(volume.host, Unset): |
There was a problem hiding this comment.
Missing regression test (the repo's SDK guidelines require one for every bug fix, and the PR checklist marks testing as "Not run"). There is no test reproducing #1658 ('Unset' object has no attribute 'path'). Please add a converter test covering to_api_volume with (a) an unset backend and (b) each backend set, then run uv run pytest.
There was a problem hiding this comment.
Hi @Pangjiping, thanks for the review! I have addressed all your comments in the latest commit. All local tests (uv run pytest) and linting (uv run ruff check) pass successfully now. Could you please take another look? Thanks!
|
|
||
| api_pvc = UNSET | ||
| if volume.pvc is not None: | ||
| if volume.pvc is not None and not isinstance(volume.pvc, Unset): |
There was a problem hiding this comment.
Concern about the root cause: the domain Volume model (opensandbox.models.sandboxes.Volume, the type shown in docs/examples) defaults host/pvc/ossfs/sub_path to None, not Unset — pydantic would reject an actual Unset value. So isinstance(volume.host, Unset) can only ever be true when callers pass the generated API Volume (opensandbox.api.lifecycle.models.Volume, whose fields default to UNSET) into create(). If that is the reported scenario, please state it in the PR and consider validating the input type (raise TypeError, as to_api_create_sandbox_request does for network_policy) instead of silently skipping, and add a repro test documenting the accepted input. Minor: new ossfs block has trailing whitespace (ruff W291).
There was a problem hiding this comment.
已补充test，当我创建携带pvc volumes的sandbox时，没有Unset判断就会报错，加上Unset判断后成功。
7508844 to
3b9064f
Compare
2 participants
Summary
#1658
Testing
Breaking Changes
Checklist