Skip to content

test(python-sdk): cover every template path-param call site on the wire #1705

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.

Sign up for GitHub

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

Closed
mishushakov wants to merge 1 commit into from
Closed

test(python-sdk): cover every template path-param call site on the wire #1705

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion packages/python-sdk/e2b/api/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,9 @@ def encode_path_param(value: str) -> str:
Endpoints that take a template ID also accept a name, which may be
namespaced (``namespace/name``) and carry a ``:tag``. Escaping those
separators keeps the value inside a single segment instead of splitting the
route, matching the JS SDK's ``encodeURIComponent``.
route, mirroring the JS SDK, where ``openapi-fetch`` runs every path param
through ``encodeURIComponent``. ``quote`` additionally escapes ``!'()*``,

@cursor cursor Bot Aug 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified this against production, and it holds — the server decodes the extra escapes and produces a byte-identical result to the encodeURIComponent form:

/templates/no-such-team%2Fte!st'a(b)c*d%3Adefault/tags
  -> 400 Invalid template reference: invalid template ID: te!st'a(b)c*d
/templates/no-such-team%2Fte%21st%27a%28b%29c%2Ad%3Adefault/tags   <- what the SDK sends
  -> 400 Invalid template reference: invalid template ID: te!st'a(b)c*d

The literal form (no-such-team/te!st'a(b)c*d:default) 404s at the request validator, so the encoding is what reaches the handler at all. The old "matching encodeURIComponent" wording really was stronger than true, and this is the accurate version.

Separately, agreeing with Codex's changeset request on this hunk: this is published packages/python-sdk source, and #1533 — a docstring-only fix in this same package — shipped with one. Add a patch changeset for @e2b/python-sdk, or drop this hunk to make the PR tests-only.

which the server decodes back to the same value.
Comment on lines +33 to +35

@chatgpt-codex-connector chatgpt-codex-connector Bot Aug 19, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Add the required Python SDK changeset

Because this commit modifies the published packages/python-sdk source, it must include a changeset for @e2b/python-sdk; without one, the repository's release tooling will not record or version this package update. Add the required changeset before merging.

AGENTS.md reference: AGENTS.md:L6-L6

Useful? React with 👍 / 👎.

@cursor cursor Bot Aug 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The divergence this sentence documents is not pinned by anything. The sibling review verified the server round-trips !'()*; the complement is that the suite cannot tell the two encodings apart either. I mutated the helper to the form the new wording invites a future reader to "simplify" to:

return quote(value, safe="!'()*")   # byte-identical to encodeURIComponent

and ran both files that touch it: 24 passed (18 call-site + 6 unit). Nothing in the repo distinguishes %21%27%28%29%2A from !'()*, in either direction — so the deliberate superset is undocumented in executable form, and a later parity change would silently be a behaviour change nobody is asked about.

One assertion in tests/shared/api/test_encode_path_param.py closes it and kills that mutant (verified: passes on head, fails on the mutant):

def test_encodes_a_superset_of_encodeuricomponent():
    # `quote(safe="")` escapes `!'()*`, which `encodeURIComponent` leaves alone.
    # The server decodes them back to the same value, so this is deliberate --
    # pin it so "match JS exactly" stays a conscious change.
    assert encode_path_param("a!b'c(d)e*f") == "a%21b%27c%28d%29e%2Af"

For the record, the character sets check out exactly as the docstring says: quote(safe="") leaves alphanumerics plus -._~, encodeURIComponent additionally leaves !'()*, and nothing else differs across printable ASCII.

"""
return quote(value, safe="")

Expand Down
212 changes: 199 additions & 13 deletions packages/python-sdk/tests/shared/template/test_build_api_path_encoding.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,27 +1,37 @@
import threading
from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer
from typing import List, Tuple

import httpx
import pytest

from e2b import AsyncSandbox, AsyncTemplate, Sandbox, Template
from e2b.api.client.client import AuthenticatedClient
from e2b.exceptions import BuildException, FileUploadException
from e2b.sandbox_async.sandbox_api import SandboxApi as AsyncSandboxApi
from e2b.sandbox_sync.sandbox_api import SandboxApi as SyncSandboxApi
from e2b.template.types import TemplateType
from e2b.template_async import build_api as async_build_api
from e2b.template_sync import build_api as sync_build_api

NAMESPACED = "my-team/my-template"
NAMESPACED_SNAPSHOT = "team-slug/my-snapshot:default"
BASE_URL = "https://api.e2b.dev"
TEMPLATE: TemplateType = {"fromImage": "ubuntu:22.04", "steps": [], "force": False}


def _handler(request: httpx.Request) -> httpx.Response:
if request.method == "DELETE":
return httpx.Response(200, json={})
# The real endpoint answers 204 with no body.
return httpx.Response(204)
if request.url.path.endswith("/tags"):
return httpx.Response(200, json=[])
return httpx.Response(404, json={"code": 404, "message": "not found"})


def _recording_client() -> tuple[AuthenticatedClient, list[httpx.Request]]:
def _recording_client() -> Tuple[AuthenticatedClient, List[httpx.Request]]:
"""Client whose requests are answered locally and recorded for assertions."""
requests: list[httpx.Request] = []
requests: List[httpx.Request] = []

def record(request: httpx.Request) -> httpx.Response:
requests.append(request)
Expand All @@ -36,54 +46,140 @@ def record(request: httpx.Request) -> httpx.Response:
return client, requests


def _request_path(request: httpx.Request) -> bytes:
"""The raw path the SDK handed to the transport, without the query string.

`raw_path` keeps the percent-encoding (`url.path` decodes it again), which
is the whole point of these assertions.
"""
return request.url.raw_path.split(b"?")[0]


def test_sync_alias_check_sends_encoded_alias():
client, requests = _recording_client()

assert sync_build_api.check_alias_exists(client, NAMESPACED) is False

# raw_path is what goes on the wire: the slash stays encoded, so the whole
# alias remains a single path segment instead of splitting the route.
assert requests[0].url.raw_path == b"/templates/aliases/my-team%2Fmy-template"
# The slash stays encoded, so the whole alias remains a single path segment
# instead of splitting the route.
assert _request_path(requests[0]) == b"/templates/aliases/my-team%2Fmy-template"


def test_sync_alias_check_leaves_plain_alias_untouched():
client, requests = _recording_client()

assert sync_build_api.check_alias_exists(client, "my-template") is False

assert requests[0].url.raw_path == b"/templates/aliases/my-template"
assert _request_path(requests[0]) == b"/templates/aliases/my-template"


def test_sync_get_template_tags_sends_encoded_template_id():
client, requests = _recording_client()

assert sync_build_api.get_template_tags(client, NAMESPACED) == []

assert requests[0].url.raw_path == b"/templates/my-team%2Fmy-template/tags"
assert _request_path(requests[0]) == b"/templates/my-team%2Fmy-template/tags"


async def test_async_alias_check_sends_encoded_alias():
client, requests = _recording_client()

assert await async_build_api.check_alias_exists(client, NAMESPACED) is False

assert requests[0].url.raw_path == b"/templates/aliases/my-team%2Fmy-template"
assert _request_path(requests[0]) == b"/templates/aliases/my-team%2Fmy-template"


async def test_async_alias_check_leaves_plain_alias_untouched():
client, requests = _recording_client()

assert await async_build_api.check_alias_exists(client, "my-template") is False

assert requests[0].url.raw_path == b"/templates/aliases/my-template"
assert _request_path(requests[0]) == b"/templates/aliases/my-template"


async def test_async_get_template_tags_sends_encoded_template_id():
client, requests = _recording_client()

assert await async_build_api.get_template_tags(client, NAMESPACED) == []

assert requests[0].url.raw_path == b"/templates/my-team%2Fmy-template/tags"
assert _request_path(requests[0]) == b"/templates/my-team%2Fmy-template/tags"


# The build calls below take the template ID the API handed back rather than a
# user-typed name, so today they cannot carry a slash. They still encode it —
# these tests keep that from silently regressing if the API starts returning
# namespaced IDs. Only the request path is under test, so the mock answers the
# 404 that makes each call raise instead of a valid response body.

@cursor cursor Bot Aug 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This choice — answering 404 so the call raises rather than fabricating a valid body — pays a dividend the description doesn't claim, and it is worth knowing about because it makes the file harder to argue away.

These three tests give first-ever coverage to the generated _parse_response error ladders of the endpoints they hit:

generated module base → head
get_templates_template_id_files_hash.py 27/44 → 32/44 lines, 1/12 → 5/12 branches
post_v_2_templates_template_id_builds_build_id.py 31/42 → 35/42, 1/8 → 5/8
get_templates_template_id_builds_build_id_status.py 35/50 → 39/50, 2/12 → 5/12

That is 13 of the PR's 18 newly-covered statements — the if response.status_code == 400/401/404 arms and the Error.from_dict(response.json()) calls behind them, which no test in the suite had ever reached. One detail worth being aware of rather than changing: for post_v_2_..._builds_build_id a 404 is not a documented response, so the mock walks off the end of the ladder into if client.raise_on_unexpected_status: → return None, and trigger_build then raises BuildException via the "no parsed body" path rather than the error path. Harmless here since only the request path is asserted (and the comment above says exactly that), but it does mean this test's exception is incidental — if you ever want it to be meaningful, 401 or 500 would exercise a documented arm.

Also incidental, not a request: hash_ and build_id are the only path params in these routes left unencoded, which is right — both are API-generated — and the plain "filehash" / "build-id" values here correctly decline to assert anything about them.



def test_sync_file_upload_link_sends_encoded_template_id():
client, requests = _recording_client()

with pytest.raises(FileUploadException):
sync_build_api.get_file_upload_link(client, NAMESPACED, "filehash")

assert (
_request_path(requests[0]) == b"/templates/my-team%2Fmy-template/files/filehash"
)


def test_sync_trigger_build_sends_encoded_template_id():
client, requests = _recording_client()

with pytest.raises(BuildException):
sync_build_api.trigger_build(client, NAMESPACED, "build-id", TEMPLATE)

assert (
_request_path(requests[0])
== b"/v2/templates/my-team%2Fmy-template/builds/build-id"
)


def test_sync_build_status_sends_encoded_template_id():
client, requests = _recording_client()

with pytest.raises(BuildException):
sync_build_api.get_build_status(client, NAMESPACED, "build-id", 0)

assert (
_request_path(requests[0])
== b"/templates/my-team%2Fmy-template/builds/build-id/status"
)


async def test_async_file_upload_link_sends_encoded_template_id():
client, requests = _recording_client()

with pytest.raises(FileUploadException):
await async_build_api.get_file_upload_link(client, NAMESPACED, "filehash")

assert (
_request_path(requests[0]) == b"/templates/my-team%2Fmy-template/files/filehash"
)


async def test_async_trigger_build_sends_encoded_template_id():
client, requests = _recording_client()

with pytest.raises(BuildException):
await async_build_api.trigger_build(client, NAMESPACED, "build-id", TEMPLATE)

assert (
_request_path(requests[0])
== b"/v2/templates/my-team%2Fmy-template/builds/build-id"
)


async def test_async_build_status_sends_encoded_template_id():
client, requests = _recording_client()

with pytest.raises(BuildException):
await async_build_api.get_build_status(client, NAMESPACED, "build-id", 0)

assert (
_request_path(requests[0])
== b"/templates/my-team%2Fmy-template/builds/build-id/status"
)


def test_sync_delete_snapshot_sends_encoded_snapshot_id(monkeypatch):
Expand All @@ -96,7 +192,7 @@ def test_sync_delete_snapshot_sends_encoded_snapshot_id(monkeypatch):

# Snapshot IDs carry both a namespace slash and a `:tag`; both must stay
# encoded so DELETE /templates/{id} targets one path segment.
assert requests[0].url.raw_path == b"/templates/team-slug%2Fmy-snapshot%3Adefault"
assert _request_path(requests[0]) == b"/templates/team-slug%2Fmy-snapshot%3Adefault"


async def test_async_delete_snapshot_sends_encoded_snapshot_id(monkeypatch):
Expand All @@ -109,4 +205,94 @@ async def test_async_delete_snapshot_sends_encoded_snapshot_id(monkeypatch):
NAMESPACED_SNAPSHOT, api_key="e2b_test"
)

assert requests[0].url.raw_path == b"/templates/team-slug%2Fmy-snapshot%3Adefault"
assert _request_path(requests[0]) == b"/templates/team-slug%2Fmy-snapshot%3Adefault"


# The tests above stop at the httpx layer, where the SDK builds the request.
# In production the request is then handed to pyqwest, which rebuilds the URL
# in reqwest — so these drive the public API through the real transport stack
# against a local server and assert the request target that actually goes out
# on the wire.


@pytest.fixture
def path_recording_server():
"""Local HTTP server yielding its base URL and the raw request targets it saw."""
paths: List[str] = []

class Handler(BaseHTTPRequestHandler):
def do_GET(self):
# `self.path` is the request target verbatim — the stdlib server
# does not percent-decode it.
paths.append(self.path)
body = b'{"code":404,"message":"not found"}'
self.send_response(404)
self.send_header("Content-Type", "application/json")
self.send_header("Content-Length", str(len(body)))
self.end_headers()
self.wfile.write(body)

def do_DELETE(self):
paths.append(self.path)
self.send_response(204)
self.send_header("Content-Length", "0")
self.end_headers()

def log_message(self, *args):
pass

server = ThreadingHTTPServer(("127.0.0.1", 0), Handler)
thread = threading.Thread(target=server.serve_forever, daemon=True)
thread.start()
try:
yield f"http://127.0.0.1:{server.server_address[1]}", paths
finally:
server.shutdown()
thread.join()

@cursor cursor Bot Aug 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

server.shutdown() stops the accept loop but does not close the listening socket — that is server_close(). Without it the file emits four ResourceWarning: unclosed <socket.socket ...> per run (one per wire test; confirmed with pytest -W error::ResourceWarning), and adding the one line silences all four with 18 still passing.

Precedent is split, so this is a nit rather than a defect: the echo_server fixture in tests/test_api_client_transport.py omits it too (and already emits 13 such warnings), while tests/{sync,async}/template_*/test_upload_file.py call server.server_close() in their teardown.

        server.shutdown()
        thread.join()
        server.server_close()



def test_sync_exists_keeps_alias_encoded_on_the_wire(
path_recording_server, test_api_key
):
api_url, paths = path_recording_server

assert Template.exists(NAMESPACED, api_key=test_api_key, api_url=api_url) is False

assert paths == ["/templates/aliases/my-team%2Fmy-template"]


async def test_async_exists_keeps_alias_encoded_on_the_wire(
path_recording_server, test_api_key
):
api_url, paths = path_recording_server

assert (
await AsyncTemplate.exists(NAMESPACED, api_key=test_api_key, api_url=api_url)
is False
)

assert paths == ["/templates/aliases/my-team%2Fmy-template"]


def test_sync_delete_snapshot_keeps_snapshot_id_encoded_on_the_wire(
path_recording_server, test_api_key
):
api_url, paths = path_recording_server

assert Sandbox.delete_snapshot(

@cursor cursor Bot Aug 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small consistency thing: every other assertion in this file pins the exact value (is False), while this one and its async twin assert truthiness. delete_snapshot returns a bool whose False branch is meaningful (404 → "nothing to delete"), so assert Sandbox.delete_snapshot(...) is True documents that the 204 path was taken rather than merely that something truthy came back.

NAMESPACED_SNAPSHOT, api_key=test_api_key, api_url=api_url
)

assert paths == ["/templates/team-slug%2Fmy-snapshot%3Adefault"]


async def test_async_delete_snapshot_keeps_snapshot_id_encoded_on_the_wire(
path_recording_server, test_api_key
):
api_url, paths = path_recording_server

assert await AsyncSandbox.delete_snapshot(
NAMESPACED_SNAPSHOT, api_key=test_api_key, api_url=api_url
)

assert paths == ["/templates/team-slug%2Fmy-snapshot%3Adefault"]
Loading