-
Notifications
You must be signed in to change notification settings - Fork 698
chore: add python type checking #1025
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
6be9d29 to
d96f488
Compare
d96f488 to
065320a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return self._get_request_timeout(self.request_timeout, request_timeout) | ||
|
|
||
| def get_sandbox_url(self, sandbox_id: str, sandbox_domain: str) -> str: | ||
| if self._sandbox_url: | ||
| return self._sandbox_url | ||
| url = self._sandbox_url() | ||
| if url is not None: | ||
| return url |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Custom sandbox_url option ignored
The constructor now assigns the supplied sandbox_url to self.sandbox_url, but get_sandbox_url() still calls the _sandbox_url() static helper and never uses the instance attribute. Callers that pass a custom sandbox_url (for example to point at a self‑hosted envd) will have their override dropped and always connect to the default host. This regresses configuration flexibility; get_sandbox_url should read the stored value before falling back to the default.
Useful? React with 👍 / 👎.
| metadata=metadata or {}, | ||
| timeout=timeout, | ||
| env_vars=env_vars or {}, | ||
| mcp=mcp or UNSET, | ||
| mcp=McpType0(**mcp) if mcp is not None else UNSET, | ||
| secure=secure, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Async sandbox MCP creation raises TypeError
When building the request body, mcp=McpType0(**mcp) is used. The generated McpType0 class has no keyword parameters (__init__ accepts none because it only stores additional_properties), so any non‑empty dict causes TypeError: __init__() got an unexpected keyword argument … before the request is sent. Creating a sandbox with an MCP configuration now crashes. Convert via McpType0.from_dict(mcp) or pass the dict through unchanged.
Useful? React with 👍 / 👎.
| metadata=metadata or {}, | ||
| timeout=timeout, | ||
| env_vars=env_vars or {}, | ||
| mcp=mcp or UNSET, | ||
| mcp=McpType0(**mcp) if mcp is not None else UNSET, | ||
| secure=secure, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sync sandbox MCP creation raises TypeError
The synchronous sandbox API mirrors the async change, calling McpType0(**mcp) when a configuration is provided. Because McpType0’s initializer does not accept arbitrary keys, any attempt to pass an MCP configuration raises TypeError and the sandbox is never created. This breaks the Sandbox.create workflow for MCP users. Use McpType0.from_dict(mcp) or pass the dictionary directly.
Useful? React with 👍 / 👎.
| def get_sandbox_url(self, sandbox_id: str, sandbox_domain: str) -> str: | ||
| if self._sandbox_url: | ||
| return self._sandbox_url | ||
| url = self._sandbox_url() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Incorrect URL access bypasses cache.
The method calls self._sandbox_url() (the static method) instead of accessing self.sandbox_url (the instance attribute). This bypasses the cached value set in __init__ at line 119, causing the environment variable to be read on every call instead of using the stored value. The instance attribute was renamed from _sandbox_url to sandbox_url but this call site wasn't updated accordingly.
| ) | ||
| token = config.access_token | ||
|
|
||
| if token is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this shouldn't be here, what if I use supabase headers for auth?
add basic pyright type checking
Note
Introduce Pyright type checking and update SDK types/optionals, auth handling, and MCP/request models with minor runtime safeguards.
pyrightto dev dependencies, Makefilelinttarget, and new[tool.pyright]config inpyproject.toml.poetry.lockaccordingly (addspyright,nodeenv; metadata tweaks).Response[Any], typed headers) and stricter auth token checks inApiClient.ConnectionConfig: exposesandbox_url, adjustget_sandbox_url()to honor env/override, and return typeddict[str, Any]fromget_api_params().sandbox_domainnon-optional at instance level (fallback to SDK domain); propagate optionaldomain/envd_access_tokenhandling when connecting/creating (checks forUnset).user; default todefault_usernamefor older envd versions (ENVD_DEFAULT_USER).McpType0(**mcp); pass UNSET when absent.res.parsed is None; adjust metrics start/end to useUNSET.resolveSymlinkshandling (explicit defaulting toRESOLVE_SYMLINKS).e2b/__init__.py(no behavior change).Written by Cursor Bugbot for commit 065320a. This will update automatically on new commits. Configure here.