promote: test → main (GET /api/sandbox/reconnect)#526
Merged
Conversation
…open-agents (#522) * feat(sandbox): port POST /api/sandbox + GET /api/sandbox/status from open-agents Implements the two session-scoped sandbox endpoints required to drive the chat "loading sandbox..." UX on session entry — matching the contract documented in recoupable/docs#192 (now merged on main). POST /api/sandbox provisions or resumes a Sandbox via the abstraction inlined in #507. When sessionId is supplied, the deterministic sandboxName ensures resume idempotency and the resolved sandbox state is persisted onto the session row (sandbox_state, lifecycle_state = "active", lifecycle_version bumped, sandbox_expires_at, last_activity_at) so subsequent GET /api/sandbox/status calls report the sandbox as active. GET /api/sandbox/status is DB-only — reads the session row, computes status as "active" when sandbox_state is set and not expired (10s buffer to match open-agents), otherwise "no_sandbox". hasSnapshot is true when snapshot_url is set. Mirrors the lifecycle envelope shape from open-agents so the frontend cutover is byte-identical. Files follow existing api conventions: - Route shells in app/api/sandbox/ delegate to handlers in lib/sandbox/ - Auth via validateAuthContext (Privy Bearer or x-api-key) - Validation via Zod (validateCreateSandboxBody) - Supabase ops in lib/supabase/sessions/ (one fn per file) - Error envelope { status: "error", error } matches sessions PRs TDD red → green: - 7 new test files added covering validator, helper, Supabase wrapper, both handlers, and the two route shells - 30 new tests, all passing (was 2461, now 2491) - pnpm lint:check clean Out of scope (deferred to follow-up PRs): - Org-snapshot lookup / kickBuildOrgSnapshotWorkflow (cold-start opt) - Skill installation (installSessionGlobalSkills) - Lifecycle workflow kick (no workflow infra in api yet) - DELETE /api/sandbox + PUT /api/sandbox/snapshot (no UI callers identified during the open-agents grep audit) - /api/sandbox/{extend,activity,reconnect,snapshot} sub-routes — to follow once these two land and the chat UX is validated Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(sandbox): treat type-stub sandbox_state as no_sandbox in /status Smoke test against the preview deployment caught a regression that defeated the entire loading-state UX this PR exists to enable: GET /api/sandbox/status reported `"active"` immediately after POST /api/sessions, before any sandbox had been provisioned. Root cause: POST /api/sessions (PR #515) inserts `sandbox_state` as the type stub `{ type: "vercel" }`. The previous `isSandboxActive` check `if (!row.sandbox_state) return false` saw a truthy object and fell through; with `sandbox_expires_at = null` (no expiry yet), the function returned true. Fix: introduce `hasRuntimeSandboxState(state)` that distinguishes the type stub from real runtime metadata by requiring a non-empty `sandboxName` (set by `getSessionSandboxName(sessionId)` in POST /api/sandbox and preserved by the abstraction's `getState()`). Mirrors open-agents' equivalent helper. TDD red → green: - Regression test pinned to the exact production scenario (sandbox_state = {type:"vercel"}, sandbox_expires_at = null, lifecycle_state = "provisioning") asserting status=no_sandbox - Companion test asserting status=active once sandboxName is set - 6 unit tests for the new helper covering null/undefined, scalars, type stub, populated state, and empty-string sandboxName edge case - Confirmed RED before implementing, GREEN after - Suite: 2491 → 2499 (+8 new tests), pnpm lint:check clean Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(sandbox): SRP/KISS extractions + Tier 1 correctness fixes Addresses review feedback on PR #522 and the "missing from open-agents" audit: User-flagged review comments: - SRP: extract `buildSource` to lib/sandbox/buildSource.ts - YAGNI: drop `isNewBranch` from POST /api/sandbox — chat never sets it (note: docs PR #192 still documents it; will open follow-up docs PR to drop from sandbox.json) - SRP: extract `isoToEpochMs` to lib/sandbox/isoToEpochMs.ts - SRP: extract `buildLifecycle` to lib/sandbox/buildLifecycle.ts - SRP: extract `isSandboxActive` to lib/sandbox/isSandboxActive.ts - KISS: rename lib/supabase/sessions/updateSessionSandboxState.ts -> updateSession.ts, generalize signature to (id, TablesUpdate<"sessions">) Tier 1 correctness gaps from the open-agents comparison: 1. GitHub URL validation via parseGitHubRepoUrl in validateCreateSandboxBody — bad URLs now return a clean 400 instead of falling through to a confusing 502 from the sandbox provider 2. Service GitHub token plumbed into connectSandbox options via new lib/github/getServiceGithubToken.ts — private repos can now clone 3. snapshot_url + snapshot_created_at cleared on fresh provision so GET /api/sandbox/status no longer surfaces stale snapshot URLs from prior runs TDD red -> green: - 5 new unit-test files for the extracted helpers (buildSource, isoToEpochMs, buildLifecycle, isSandboxActive, getServiceGithubToken) - updateSession.test.ts replaces updateSessionSandboxState.test.ts - Updated validator + handler tests for the contract changes (drop isNewBranch, add bad-URL 400 cases, assert githubToken plumbing, assert snapshot_url/snapshot_created_at: null in update payload) - Confirmed RED before each implementation - Suite: 2499 -> 2516 (+17 net new tests), pnpm lint:check clean Files net delta: -241 / +70 lines (extractions + handler shrinks) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(sandbox): drop branch from POST /api/sandbox contract YAGNI/KISS per review feedback — chat always works off the repo's default branch, so the explicit `branch` input adds no value. - Drop `branch` from createSandboxBodySchema - Inline the now-trivial source object in createSandboxHandler (`{ repo: body.repoUrl }`) and delete `lib/sandbox/buildSource.ts` + its test - Read `currentBranch` for the response from the sandbox handle's own `currentBranch` property (whatever the SDK actually checked out), falling back to "main" — no longer derives from a request field that no longer exists Tests: TDD red -> green. - Validator test asserts `branch` is stripped from the body even if a client still sends it - Handler test asserts `currentBranch` in the response comes from `sandbox.currentBranch` (mocked to "release/v2") not from input - Suite stays at 2516 (-1 from buildSource.test deletion +1 new currentBranch test) Pairs with docs PR recoupable/docs#194 (merged) which already removed `branch` and `isNewBranch` from the published OpenAPI spec. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(sandbox): port GET /api/sandbox/reconnect from open-agents Implements the live runtime probe endpoint required for the chat loading-UX cutover on session re-entry / tab refocus. Matches the contract documented in recoupable/docs#195 (now merged on main). Unlike GET /api/sandbox/status (DB-only read), /reconnect actually runs `sandbox.exec("pwd")` inside the runtime so the UI can distinguish a truly-alive sandbox from a DB row whose sandbox no longer exists. Behavior: - 200 status="no_sandbox" when sandbox_state lacks runtime metadata (delegates to hasRuntimeSandboxState — the same gate /status uses) - 200 status="connected" with sandbox.expiresAt when the probe succeeds - 200 status="expired" when the probe throws — also clears sandbox_state on the session row and sets lifecycle_state to "hibernated" so subsequent /status reads agree with the probe - hasSnapshot derived from snapshot_url across all three outcomes - 4xx for auth (401), missing sessionId (400), forbidden (403), not-found (404) — same envelope as /status Files follow the existing api conventions established by PR #522: - app/api/sandbox/reconnect/route.ts: thin GET delegation + OPTIONS - lib/sandbox/getSandboxReconnectHandler.ts: handler logic - Reuses validateAuthContext, selectSessions, hasRuntimeSandboxState, buildLifecycle, connectSandbox, updateSession - Error envelope { status, error } matches sessions PRs TDD red -> green: - Handler tests cover auth fail, missing sessionId, 404, 403, no_sandbox, hasSnapshot derivation, connected (with expiresAt), expired (with state-clear assertion), and lifecycle envelope shape - Thin route shell test asserts delegation - Suite: 2516 -> 2526 (+10 net new tests), pnpm lint:check clean Out of scope (deferred per the gap analysis): - Transient vs unavailable error distinction. open-agents preserves runtime state on transient errors (network blip != dead sandbox). v1 treats every probe failure as expired, which is safer for the loading UX (user can retry). Worth porting once we have a real signal that this is happening in production. - Stale-state lifecycle workflow kick (open-agents' /status has it too — needs workflow infra in api). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(sandbox): extract noSandboxResponse to its own file (SRP) Per review feedback on PR #525 — pulls the inline `noSandboxResponse` helper out of `getSandboxReconnectHandler.ts` into its own file so it can be reused by future endpoints (e.g., `/snapshot` resume) and so the handler file stops carrying response-shape construction logic. The narrowed `ReconnectBody` type in the handler now only covers the two outcomes the handler actually constructs locally (`connected` / `expired`); the `no_sandbox` shape lives with its builder. TDD red -> green: 3 unit tests for the extracted helper covering 200 status, hasSnapshot derivation, and lifecycle envelope projection. Suite: 2526 -> 2529. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(sandbox): extract validateSandboxReconnectRequest + fix preview build Two changes bundled: 1) SRP per review feedback (sweetmantech) — extract the auth + sessionId-from-query + session lookup + ownership check pre-flight from `getSandboxReconnectHandler` into its own `validateSandboxReconnectRequest.ts`. Mirrors the `validateCreateSandboxBody` pattern: returns either a 4xx NextResponse describing the first failure, or `{ row, auth }` for the handler to consume. 2) Type fix for `next build` — `connectSandbox(row.sandbox_state as SandboxState)` failed to compile against `Json` (union includes primitives + arrays); cast through `unknown` first. The `hasRuntimeSandboxState` gate above ensures the runtime shape is safe at the call site, so the double cast is justified — comment added explaining why. The vitest pass alone wasn't enough to catch the type error — `next build` runs a separate `tsc` step that the test runner skips. Caught by the Vercel preview build failing on the previous commit. TDD red -> green: - 5 unit tests for the new validator covering auth fail (passes through), missing sessionId (400), session not found (404), ownership mismatch (403), and happy-path return shape ({row, auth}) - Existing handler tests pass unchanged — the module-level mocks for validateAuthContext / selectSessions still intercept the calls now that they're behind the new validator - Suite: 2529 -> 2534, pnpm lint:check clean Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (4)
✨ Finishing Touches🧪 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 |
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.
Promotes the sandbox reconnect endpoint from
testtomain.What lands on main
feat(sandbox): port GET /api/sandbox/reconnect from open-agents(feat(sandbox): port GET /api/sandbox/reconnect from open-agents #525, squash-merged into test as432b8ab9)sandbox.exec("pwd")distinguishesconnected/expired/no_sandboxexpired: clearssandbox_state+ setslifecycle_state: "hibernated"so subsequent/statusreads agree with the probevalidateSandboxReconnectRequest(auth + sessionId + ownership pre-flight) andnoSandboxResponseextractions per review feedbackVerification
no_sandbox/connectedpaths all match spec (comment)🤖 Generated with Claude Code
Summary by cubic
Promotes
GET /api/sandbox/reconnectfromtesttomain, adding a live runtime probe so the app can tell if a session’s sandbox is connected, expired, or missing. Keeps session lifecycle in sync with the probe result.New Features
GET /api/sandbox/reconnect; runssandbox.exec("pwd")to returnconnected,expired, orno_sandbox.expired, clearssandbox_stateand setslifecycle_state: "hibernated"so/statusmatches reality.hasSnapshotand a lifecycle envelope; includesexpiresAtwhen connected.OPTIONSand disables caching (force-no-store).Refactors
validateSandboxReconnectRequest(auth,sessionId, ownership) andnoSandboxResponse.Json→SandboxStateviaunknown. Adds focused tests for handler, validator, and response.Written for commit 432b8ab. Summary will update on new commits.