fix(sdk): remove /v1 prefix from ConnectionConfig.get_base_url()#1045
fix(sdk): remove /v1 prefix from ConnectionConfig.get_base_url()#1045ashishpatel26 wants to merge 2 commits into
Conversation
…ix 504 timeout
ConnectionConfig.get_base_url() was unconditionally appending /_API_VERSION ("v1")
to the domain, causing all SDK requests to hit /v1/sandboxes instead of /sandboxes.
In deployments where the gateway routes /v1/* to a synchronous blocking endpoint,
this caused 504 Gateway Timeout when pod startup exceeded the proxy idle timeout (~60s).
The generated API client already uses correct relative paths (e.g. /sandboxes), so
the base URL should be the plain domain without any version prefix. The server
registers routes at both / and /v1 with the same async handler, so removing /v1
from the base URL routes traffic to the non-prefixed async endpoint that returns
202 Accepted immediately and lets the SDK poll for readiness via check_ready().
Fixes opensandbox-group#591
ashishpatel26
requested review from
Pangjiping,
hittyt,
jwx0925 and
ninan-nn
as code owners
|
Changed directories: sdks. 📋 Recommended labels (based on changed files):
Other available labels:
💡 Tip: Use |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR updates the Python sandbox SDK connection configuration so get_base_url() no longer appends the /v1 prefix, preventing requests from being routed to the blocking /v1/sandboxes endpoint (linked to issue #591 / 504 timeouts).
Changes:
- Remove automatic
/v1suffixing fromget_base_url()implementations. - Strip trailing slashes when the domain already includes a scheme.
- Expand test coverage to assert base URL behavior across direct/proxy modes and trailing-slash inputs.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| sdks/sandbox/python/tests/test_connection_config.py | Updates and expands tests to assert get_base_url() returns a non-versioned base URL and strips trailing slashes. |
| sdks/sandbox/python/src/opensandbox/config/connection_sync.py | Removes /v1 from sync get_base_url() and documents rationale; strips trailing slash when domain includes scheme. |
| sdks/sandbox/python/src/opensandbox/config/connection.py | Removes /v1 from async get_base_url() and documents rationale; strips trailing slash when domain includes scheme. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if domain.startswith("http://") or domain.startswith("https://"): | ||
| return domain.rstrip("/") | ||
| return f"{self.protocol}://{domain}" |
| domain = self.get_domain() | ||
| if domain.startswith("http://") or domain.startswith("https://"): | ||
| return f"{domain}/{self._API_VERSION}" | ||
| return f"{self.protocol}://{domain}/{self._API_VERSION}" | ||
| return domain.rstrip("/") | ||
| return f"{self.protocol}://{domain}" |
| def test_get_base_url_trailing_slash_stripped() -> None: | ||
| """Trailing slashes on the domain are stripped to prevent double-slash in paths.""" | ||
| cfg = ConnectionConfig(domain="http://sandbox-api.example.com/") | ||
| assert cfg.get_base_url() == "http://sandbox-api.example.com" |
![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: 968fc55f34
ℹ️ 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 f"{self.protocol}://{domain}/{self._API_VERSION}" | ||
| if domain.startswith("http://") or domain.startswith("https://"): | ||
| return domain.rstrip("/") | ||
| return f"{self.protocol}://{domain}" |
There was a problem hiding this comment.
For configurations that provide only the host (for example the default localhost:8080 or OPEN_SANDBOX_DOMAIN=api.example.com), this now sends lifecycle and diagnostics calls to /sandboxes instead of the public /v1/sandboxes API. I checked the generated Python client and its paths are unversioned (post_sandboxes.py uses "url": "/sandboxes"), while the OpenAPI specs still define the server URL as http://localhost:8080/v1 and the other SDKs document/construct lifecycle base URLs with /v1; deployments or gateways that expose only the versioned API will 404 these Python SDK calls. If a hosted gateway needs the unversioned path to avoid the blocking route, that needs to be an explicit base URL/domain choice rather than changing the default host-only behavior.
Useful? React with 👍 / 👎.
|
Thanks for the reviews. Addressing feedback: Codex P2 – Breaking lifecycle/diagnostics base path for host-only configs: Valid concern. When Copilot – Trailing slash normalization: Valid edge case. Will add Copilot – Missing test case for trailing-slash domain: Will add a test for |
…ormats
Add rstrip('/') before URL construction so domains with trailing
slashes (e.g. 'api.example.com/') don't produce double-slash joins
in the generated client paths. Also add test coverage for the
no-scheme trailing-slash case.
Summary
ConnectionConfig.get_base_url()(async and sync) appended/v1to the domain, so all SDK requests went to/v1/sandboxes/.../v1/prefix routes through a blocking path — when sandbox startup exceeds the gateway idle timeout (~60 s), the connection is dropped with a 504Test plan
uv run pytest sdks/sandbox/python/tests/test_connection_config.py— updated + 3 new tests verify no/v1in base URL for both proxy and direct modesFixes #591