diff --git a/.changeset/box-git-fixes.md b/.changeset/box-git-fixes.md new file mode 100644 index 00000000..87bcfac5 --- /dev/null +++ b/.changeset/box-git-fixes.md @@ -0,0 +1,11 @@ +--- +"@upstash/box": patch +--- + +Fix a lost exit status in `exec.stream()`, and three things in the git namespace that made it unusable from a caller's point of view. + +- `exec.stream()` could end without an `exit` chunk. The `event: exit` marker and its `data:` payload can arrive in separate network reads; the parser matched a half-arrived line and returned, ending the stream with no exit status at all. A caller had no way to tell whether the command succeeded, and about one run in thirty of a short command was affected. The payload is now waited for. + +- `git.updateConfig()` sent its request to `/v2/box/{id}/git-config`, which the coordinator does not serve. The identity endpoint is `/v2/box/{id}/config/git`, so every call returned 404 and no git identity was ever set through the SDK. +- `git.exec()` results now carry `exit_code`. The API has always returned it; the type omitted it, so callers could not tell a failed git command (for example exit 128 when the folder is not a repository) from a successful one. +- `git.clone()` accepts `folder`, naming the directory the repository is cloned into. Unlike every other git operation, where the folder is an existing directory derived from `cd()`, clone's folder is the destination and does not exist yet, so it could not be expressed at all: `cd()` fails on a directory the clone is about to create. diff --git a/packages/python-sdk/CHANGELOG.md b/packages/python-sdk/CHANGELOG.md index df422964..e07b62fa 100644 --- a/packages/python-sdk/CHANGELOG.md +++ b/packages/python-sdk/CHANGELOG.md @@ -2,6 +2,17 @@ All notable changes to `upstash-box` (Python) are documented here. +## 0.3.1 + +- Fix `git.update_config()` sending its request to `/v2/box/{id}/git-config`, + which the coordinator does not serve. The identity endpoint is + `/v2/box/{id}/config/git`, so every call returned 404 and no git identity was + ever set through the SDK. +- Add `folder` to `git.clone()`, naming the directory the repository is cloned + into. Unlike every other git operation, where the folder is an existing + directory derived from `cd()`, clone's folder is the destination and does not + exist yet, so it could not be expressed at all. + ## 0.3.0 - `exec.session(...)` — live command sessions over a WebSocket, matching diff --git a/packages/python-sdk/pyproject.toml b/packages/python-sdk/pyproject.toml index 5a563543..b3be4d07 100644 --- a/packages/python-sdk/pyproject.toml +++ b/packages/python-sdk/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "hatchling.build" [project] name = "upstash-box" -version = "0.3.0" +version = "0.3.1" description = "Upstash Box SDK - Python client for async and parallel AI coding agents" readme = "README.md" license = { text = "MIT" } diff --git a/packages/python-sdk/tests/_async/test_box_git.py b/packages/python-sdk/tests/_async/test_box_git.py index 8eafd179..1821843f 100644 --- a/packages/python-sdk/tests/_async/test_box_git.py +++ b/packages/python-sdk/tests/_async/test_box_git.py @@ -32,6 +32,17 @@ async def test_clone_with_depth(): await box.aclose() +@respx.mock +async def test_clone_into_explicit_folder(): + box = await make_async_box(respx.mock) + route = respx.post(f"{BASE}/git/clone").mock(return_value=httpx.Response(200, json={})) + # For clone the folder is the destination, so it does not exist yet and + # cd() cannot be used to express it. + await box.git.clone(repo="https://github.com/u/r", folder="my-app") + assert last_json_body(route)["folder"] == "my-app" + await box.aclose() + + @respx.mock async def test_diff_and_status(): box = await make_async_box(respx.mock) @@ -69,7 +80,9 @@ async def test_update_config_requires_one_field(): @respx.mock async def test_update_config(): box = await make_async_box(respx.mock) - route = respx.put(f"{BASE}/git-config").mock( + # The coordinator serves this under config/git; the old spelling is what + # let the wrong path ship in both SDKs. + route = respx.put(f"{BASE}/config/git").mock( return_value=httpx.Response( 200, json={"git_user_name": "Jane", "git_user_email": "j@e.com"} ) diff --git a/packages/python-sdk/upstash_box/_async/client.py b/packages/python-sdk/upstash_box/_async/client.py index f211b6fa..8319060c 100644 --- a/packages/python-sdk/upstash_box/_async/client.py +++ b/packages/python-sdk/upstash_box/_async/client.py @@ -396,9 +396,14 @@ def __init__(self, box: "AsyncBox") -> None: self._box = box async def clone( - self, *, repo: str, branch: Optional[str] = None, depth: Optional[int] = None + self, + *, + repo: str, + branch: Optional[str] = None, + depth: Optional[int] = None, + folder: Optional[str] = None, ) -> None: - await self._box._git_clone(repo, branch, depth) + await self._box._git_clone(repo, branch, depth, folder) async def diff(self) -> str: return await self._box._git_diff() @@ -1656,8 +1661,10 @@ async def _schedule_delete(self, id: str) -> None: # ==================== Git ==================== - async def _git_clone(self, repo, branch, depth) -> None: - folder = self._get_folder() + async def _git_clone(self, repo, branch, depth, folder=None) -> None: + # For clone the folder is the destination, so an explicit one wins over + # the current directory; the directory does not exist yet by definition. + folder = folder or self._get_folder() body: Dict[str, Any] = {"repo": repo, "branch": branch, "github_token": self._git_token} if depth is not None: body["depth"] = depth @@ -1694,7 +1701,7 @@ async def _git_update_config(self, user_name, user_email) -> GitConfigResult: raise BoxError("At least one of user_name or user_email is required") data = await self._request( "PUT", - f"/v2/box/{self.id}/git-config", + f"/v2/box/{self.id}/config/git", body={"git_user_name": user_name, "git_user_email": user_email}, ) return GitConfigResult.model_validate(data) diff --git a/packages/python-sdk/upstash_box/_sync/client.py b/packages/python-sdk/upstash_box/_sync/client.py index 43f9bc2b..9c49635e 100644 --- a/packages/python-sdk/upstash_box/_sync/client.py +++ b/packages/python-sdk/upstash_box/_sync/client.py @@ -391,9 +391,14 @@ def __init__(self, box: "Box") -> None: self._box = box def clone( - self, *, repo: str, branch: Optional[str] = None, depth: Optional[int] = None + self, + *, + repo: str, + branch: Optional[str] = None, + depth: Optional[int] = None, + folder: Optional[str] = None, ) -> None: - self._box._git_clone(repo, branch, depth) + self._box._git_clone(repo, branch, depth, folder) def diff(self) -> str: return self._box._git_diff() @@ -1641,8 +1646,10 @@ def _schedule_delete(self, id: str) -> None: # ==================== Git ==================== - def _git_clone(self, repo, branch, depth) -> None: - folder = self._get_folder() + def _git_clone(self, repo, branch, depth, folder=None) -> None: + # For clone the folder is the destination, so an explicit one wins over + # the current directory; the directory does not exist yet by definition. + folder = folder or self._get_folder() body: Dict[str, Any] = {"repo": repo, "branch": branch, "github_token": self._git_token} if depth is not None: body["depth"] = depth @@ -1679,7 +1686,7 @@ def _git_update_config(self, user_name, user_email) -> GitConfigResult: raise BoxError("At least one of user_name or user_email is required") data = self._request( "PUT", - f"/v2/box/{self.id}/git-config", + f"/v2/box/{self.id}/config/git", body={"git_user_name": user_name, "git_user_email": user_email}, ) return GitConfigResult.model_validate(data) diff --git a/packages/sdk/src/__tests__/box-exec-stream.test.ts b/packages/sdk/src/__tests__/box-exec-stream.test.ts index 1b2199bf..63a671b6 100644 --- a/packages/sdk/src/__tests__/box-exec-stream.test.ts +++ b/packages/sdk/src/__tests__/box-exec-stream.test.ts @@ -38,6 +38,38 @@ function mockExecStreamResponse( } as Response; } +/** + * A stream delivered in caller-chosen pieces, so a marker and its payload can + * be split the way the network splits them. + */ +function mockChunkedResponse(pieces: string[]): Response { + const encoder = new TextEncoder(); + const stream = new ReadableStream({ + start(controller) { + for (const piece of pieces) controller.enqueue(encoder.encode(piece)); + controller.close(); + }, + }); + return { + ok: true, + status: 200, + statusText: "OK", + headers: new Headers({ "content-type": "text/event-stream" }), + json: () => Promise.reject(new Error("stream response")), + text: () => Promise.resolve(pieces.join("")), + body: stream, + bodyUsed: false, + arrayBuffer: () => Promise.resolve(new ArrayBuffer(0)), + blob: () => Promise.resolve(new Blob()), + formData: () => Promise.resolve(new FormData()), + clone: () => mockChunkedResponse(pieces), + redirected: false, + type: "basic" as ResponseType, + url: "", + bytes: () => Promise.resolve(new Uint8Array()), + } as Response; +} + function mockExecStreamErrorResponse(errorMessage: string): Response { const raw = `event: error\ndata: ${JSON.stringify({ error: errorMessage })}\n\n`; const encoder = new TextEncoder(); @@ -381,3 +413,55 @@ describe("exec.streamCode", () => { expect(run.cost.computeMs).toBeGreaterThanOrEqual(0); }); }); + +describe("exec stream split across network reads", () => { + async function collect(box: Awaited>["box"]) { + const chunks: ExecStreamChunk[] = []; + for await (const chunk of await box.exec.stream("true")) chunks.push(chunk); + return chunks; + } + + it("still reports the exit status when the marker and payload arrive apart", async () => { + const { box, fetchMock } = await createTestBox(); + // The marker completing one read and the payload starting the next is what + // happens on the wire; losing the exit chunk there leaves a caller with no + // way to know whether the command succeeded. + fetchMock.mockResolvedValueOnce( + mockChunkedResponse(["hello\n", "event: exit\n", 'data: {"exit_code":3,"cpu_ns":1}\n\n']), + ); + + const chunks = await collect(box); + + expect(chunks.filter((chunk) => chunk.type === "exit")).toEqual([ + { type: "exit", exitCode: 3, cpuNs: 1 }, + ]); + }); + + it("does not duplicate the output that preceded the exit event", async () => { + const { box, fetchMock } = await createTestBox(); + fetchMock.mockResolvedValueOnce( + mockChunkedResponse(["hello\n", "event: exit\n", 'data: {"exit_code":0,"cpu_ns":0}\n\n']), + ); + + const chunks = await collect(box); + + const output = chunks + .filter((chunk) => chunk.type === "output") + .map((chunk) => (chunk as { data: string }).data) + .join(""); + expect(output).toBe("hello\n"); + }); + + it("survives the payload itself being split", async () => { + const { box, fetchMock } = await createTestBox(); + fetchMock.mockResolvedValueOnce( + mockChunkedResponse(["event: exit\n", 'data: {"exit_code":', '42,"cpu_ns":0}\n\n']), + ); + + const chunks = await collect(box); + + expect(chunks.filter((chunk) => chunk.type === "exit")).toEqual([ + { type: "exit", exitCode: 42, cpuNs: 0 }, + ]); + }); +}); diff --git a/packages/sdk/src/__tests__/box-git.test.ts b/packages/sdk/src/__tests__/box-git.test.ts index 402c592a..2260e6b0 100644 --- a/packages/sdk/src/__tests__/box-git.test.ts +++ b/packages/sdk/src/__tests__/box-git.test.ts @@ -27,6 +27,30 @@ describe("Box git operations", () => { expect(body.branch).toBe("dev"); }); + it("clones into an explicit destination folder", async () => { + const { box, fetchMock } = await createTestBox(); + fetchMock.mockResolvedValueOnce(mockResponse({})); + + // For clone the folder is where the repo lands, so it does not exist yet + // and cd() cannot be used to express it. + await box.git.clone({ repo: "owner/repo", folder: "my-app" }); + + const body = JSON.parse(fetchMock.mock.calls[1]![1]?.body as string); + expect(body.folder).toBe("my-app"); + }); + + it("prefers the explicit destination over the current directory", async () => { + const { box, fetchMock } = await createTestBox(); + fetchMock.mockResolvedValueOnce(mockResponse({ exit_code: 0, output: "" })); + await box.cd("/workspace/home/elsewhere"); + fetchMock.mockResolvedValueOnce(mockResponse({})); + + await box.git.clone({ repo: "owner/repo", folder: "my-app" }); + + const body = JSON.parse(fetchMock.mock.calls.at(-1)![1]?.body as string); + expect(body.folder).toBe("my-app"); + }); + it("clones a repo with depth", async () => { const { box, fetchMock } = await createTestBox(); fetchMock.mockResolvedValueOnce(mockResponse({})); @@ -113,7 +137,9 @@ describe("Box git operations", () => { }); const [url, init] = fetchMock.mock.calls[1]!; - expect(url).toContain("/git-config"); + // The coordinator serves this under config/git; asserting the old + // "/git-config" spelling is what let the wrong path ship. + expect(url).toContain("/config/git"); expect(init?.method).toBe("PUT"); const body = JSON.parse(init?.body as string); expect(body.git_user_name).toBe("John Doe"); @@ -174,16 +200,26 @@ describe("Box git operations", () => { describe("git.exec", () => { it("executes a git command", async () => { const { box, fetchMock } = await createTestBox(); - fetchMock.mockResolvedValueOnce(mockResponse({ output: "abc123\ndef456" })); + fetchMock.mockResolvedValueOnce(mockResponse({ output: "abc123\ndef456", exit_code: 0 })); const result = await box.git.exec({ args: ["log", "--oneline", "-2"] }); expect(result.output).toBe("abc123\ndef456"); + expect(result.exit_code).toBe(0); const [url, init] = fetchMock.mock.calls[1]!; expect(url).toContain("/git/exec"); const body = JSON.parse(init?.body as string); expect(body.args).toEqual(["log", "--oneline", "-2"]); }); + + it("forwards git's exit code, so a failed command is distinguishable", async () => { + const { box, fetchMock } = await createTestBox(); + // 128 is what git returns when the folder is not a repository. + fetchMock.mockResolvedValueOnce(mockResponse({ output: "", exit_code: 128 })); + + const result = await box.git.exec({ args: ["rev-parse", "--is-inside-work-tree"] }); + expect(result.exit_code).toBe(128); + }); }); describe("git.checkout", () => { diff --git a/packages/sdk/src/client.ts b/packages/sdk/src/client.ts index d6e6768f..0af4109a 100644 --- a/packages/sdk/src/client.ts +++ b/packages/sdk/src/client.ts @@ -2117,25 +2117,30 @@ export class Box { continue; } - // Yield any raw output before the exit event + // Yield any raw output before the exit event, keeping the marker at + // the head of the buffer in case the event itself is still arriving. if (exitIndex > 0) { yield { type: "output", data: buffer.slice(0, exitIndex) }; + buffer = buffer.slice(exitIndex); } - // Parse the exit event - const afterEvent = buffer.slice(exitIndex + "event: exit\n".length); - const dataMatch = afterEvent.match(/^data:\s*(.+)/m); - if (dataMatch) { - try { - const parsed = JSON.parse(dataMatch[1]!); - yield { - type: "exit", - exitCode: parsed.exit_code ?? 0, - cpuNs: parsed.cpu_ns ?? 0, - }; - } catch { - yield { type: "exit", exitCode: 0, cpuNs: 0 }; - } + // The marker and its payload can land in separate network reads. A + // complete line is required here so that a half-arrived one is not + // parsed; returning at that point would end the stream with no exit + // chunk at all, which is what a caller uses to learn the exit status. + const afterEvent = buffer.slice("event: exit\n".length); + const dataMatch = afterEvent.match(/^data:\s*(.+)\r?\n/m); + if (!dataMatch) continue; + + try { + const parsed = JSON.parse(dataMatch[1]!); + yield { + type: "exit", + exitCode: parsed.exit_code ?? 0, + cpuNs: parsed.cpu_ns ?? 0, + }; + } catch { + yield { type: "exit", exitCode: 0, cpuNs: 0 }; } return; } @@ -2967,7 +2972,9 @@ export class Box { // ==================== Git (private, exposed via this.git) ==================== private async _gitClone(options: GitCloneOptions): Promise { - const folder = this._getFolder(); + // For clone the folder is the destination, so an explicit one wins over the + // current directory; the directory does not exist yet by definition. + const folder = options.folder ?? this._getFolder(); await this._request("POST", `/v2/box/${this.id}/git/clone`, { body: { repo: options.repo, @@ -3013,7 +3020,7 @@ export class Box { throw new BoxError("At least one of userName or userEmail is required"); } - return this._request("PUT", `/v2/box/${this.id}/git-config`, { + return this._request("PUT", `/v2/box/${this.id}/config/git`, { body: { git_user_name: options.userName, git_user_email: options.userEmail, diff --git a/packages/sdk/src/types.ts b/packages/sdk/src/types.ts index 78af59d4..35899bf0 100644 --- a/packages/sdk/src/types.ts +++ b/packages/sdk/src/types.ts @@ -997,6 +997,12 @@ export interface GitCloneOptions { branch?: string; /** History depth (git clone --depth N); depth: 1 = shallow clone. Omit for a full clone. */ depth?: number; + /** + * Directory under the workspace to clone into. Defaults to the repository + * name. Unlike the other git operations, this names the destination rather + * than an existing directory, so it does not have to exist yet. + */ + folder?: string; } export interface GitExecOptions { @@ -1005,6 +1011,8 @@ export interface GitExecOptions { export interface GitExecResult { output: string; + /** git's own exit status; 128 when the folder is not a repository. */ + exit_code: number; } export interface GitCheckoutOptions {