add sandbox start and stop operations - #14
Merged
Conversation
📝 WalkthroughWalkthroughAdds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/services/sandboxes-client.test.ts (1)
201-209: Strengthen this test with an explicit assertion.The test currently has no
assert.*calls — it only proves thatlistSandboxesParamsSchema.parsedoesn't throw on"stopping"/"stopped". A future regression wherelist()silently no-ops, or where the state filter is dropped before being sent, would still let this test pass.Consider capturing the calls and asserting the
statequery param is forwarded:♻️ Suggested strengthening
test("sandboxes list accepts stopped lifecycle filters", async () => { - const { transport } = createRecordedTransport({ + const { transport, calls } = createRecordedTransport({ - requestJson: () => Promise.resolve({ items: [], total_items: 0 }), + requestJson: (path: string, init: RequestInit, options: unknown) => { + calls.push({ path, init, options: options as never }); + return Promise.resolve({ items: [], total_items: 0 }); + }, }); const client = new SandboxesClient(transport as never); await client.list({ state: "stopping" }); await client.list({ state: "stopped" }); + + assert.equal((calls[0]?.options as { query?: { state?: string } }).query?.state, "stopping"); + assert.equal((calls[1]?.options as { query?: { state?: string } }).query?.state, "stopped"); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/services/sandboxes-client.test.ts` around lines 201 - 209, The test currently only ensures no exception; update it to assert the outgoing request includes the state query param so we catch regressions where the filter is dropped. Use the recorded transport from createRecordedTransport (capture its recorded calls or last request) after each await client.list({ state: "stopping" }) and await client.list({ state: "stopped" }) and add assertions that the corresponding recorded request's query/state (or queryParams) equals "stopping" and "stopped". Reference the SandboxesClient.list call and the createRecordedTransport/transport recorded calls to locate where to add these assertions.src/services/sandboxes.ts (1)
217-231:start()reusesoptionsacross two requests — timeout semantics and partial-failure behavior deserve a closer look.A couple of things worth considering:
- Timeout is applied per-call. If a caller passes
{ timeout: 5000 }, both the POST/startand the follow-up GET get their own 5s budget, so worst-case wall time is ~2× what the caller expects. Consider either documenting this, splitting the budget, or sharing anAbortSignal/deadline across both calls.- Partial failure swallows a successful start. If the POST succeeds (sandbox is starting server-side) but the follow-up GET fails (network blip, transient 5xx, or the user's timeout fires on the GET), the caller sees
"Failed to start sandbox: ..."even though the start actually happened. At minimum, calling this out in the JSDoc would prevent surprised users from re-invokingstart(); a more robust fix would let GET failures fall back to returning the POST response (if it carries a body) or a minimal handle.- Asymmetry vs.
stop().stop()uses the POST response body directly;start()discards the POST response and refetches. If that's because the start endpoint doesn't return a sandbox body, a one-line code comment makes the intent obvious to future maintainers.Could you confirm the API contract for
POST /v1/sandbox/{id}/start— specifically whether it returns the sandbox body (in which case the follow-up GET could be dropped) or only an ack? That answer drives whether this refactor is worth doing or just documenting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/sandboxes.ts` around lines 217 - 231, The start() method uses the same RequestOptions for both the POST `/v1/sandbox/{id}/start` and the subsequent GET, which doubles caller timeout budget and may hide partial success (POST succeeded but GET failed); update start() to either (1) create and share a single AbortSignal/deadline across both transport.requestJson calls so the caller's timeout is a single budget, or (2) if the POST response contains the sandbox body, use that response instead of issuing the GET and only fall back to GET on missing body; also add a one-line comment explaining why POST body is/isn't sufficient (mirroring stop() behavior). Reference start(), stop(), transport.requestJson, sandboxIdOf(), normalize(), sandboxDataSchema and this.wrap() when making the change and ensure JSDoc documents timeout/partial-failure semantics if you choose to keep the two-call approach.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/services/sandboxes.ts`:
- Around line 217-231: The start() method uses the same RequestOptions for both
the POST `/v1/sandbox/{id}/start` and the subsequent GET, which doubles caller
timeout budget and may hide partial success (POST succeeded but GET failed);
update start() to either (1) create and share a single AbortSignal/deadline
across both transport.requestJson calls so the caller's timeout is a single
budget, or (2) if the POST response contains the sandbox body, use that response
instead of issuing the GET and only fall back to GET on missing body; also add a
one-line comment explaining why POST body is/isn't sufficient (mirroring stop()
behavior). Reference start(), stop(), transport.requestJson, sandboxIdOf(),
normalize(), sandboxDataSchema and this.wrap() when making the change and ensure
JSDoc documents timeout/partial-failure semantics if you choose to keep the
two-call approach.
In `@tests/services/sandboxes-client.test.ts`:
- Around line 201-209: The test currently only ensures no exception; update it
to assert the outgoing request includes the state query param so we catch
regressions where the filter is dropped. Use the recorded transport from
createRecordedTransport (capture its recorded calls or last request) after each
await client.list({ state: "stopping" }) and await client.list({ state:
"stopped" }) and add assertions that the corresponding recorded request's
query/state (or queryParams) equals "stopping" and "stopped". Reference the
SandboxesClient.list call and the createRecordedTransport/transport recorded
calls to locate where to add these assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 545d1616-c065-4455-a56a-a6af2514982b
📒 Files selected for processing (6)
src/client/sandbox.tssrc/models/sandbox.tssrc/services/sandboxes.tstests/client/client-sandbox.test.tstests/client/index-exports.test.tstests/services/sandboxes-client.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
SandboxesClient.stop()andSandboxesClient.start()plus boundSandboxinstance helpersstoppingandstoppedSummary by CodeRabbit
stopping,stopped) for improved visibility into sandbox lifecycle transitions.