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>

Author: sweetmantech

lib/github/__tests__/getServiceGithubToken.test.ts

@@ -0,0 +1,32 @@
+import{describe,it,expect,beforeEach,afterEach}from"vitest";
+import{getServiceGithubToken}from"@/lib/github/getServiceGithubToken";
+
+constORIGINAL=process.env.GITHUB_TOKEN;
+
+beforeEach(()=>{
+deleteprocess.env.GITHUB_TOKEN;
+});
+
+afterEach(()=>{
+if(ORIGINAL===undefined){
+deleteprocess.env.GITHUB_TOKEN;
+}else{
+process.env.GITHUB_TOKEN=ORIGINAL;
+}
+});
+
+describe("getServiceGithubToken",()=>{
+it("returns undefined when GITHUB_TOKEN is unset",()=>{
+expect(getServiceGithubToken()).toBeUndefined();
+});
+
+it("returns undefined when GITHUB_TOKEN is the empty string",()=>{
+process.env.GITHUB_TOKEN="";
+expect(getServiceGithubToken()).toBeUndefined();
+});
+
+it("returns the token when set",()=>{
+process.env.GITHUB_TOKEN="ghs_secret";
+expect(getServiceGithubToken()).toBe("ghs_secret");
+});
+});

lib/github/getServiceGithubToken.ts

@@ -0,0 +1,12 @@
+/**
+ * Returns the service-account GitHub token used for cloning private
+ * repositories into sandboxes. Returns undefined when the env var is
+ * unset or empty so callers can fall back to public-repo behavior
+ * without crashing.
+ *
+ * @returns The token string, or undefined.
+ */
+exportfunctiongetServiceGithubToken(): string|undefined{
+consttoken=process.env.GITHUB_TOKEN;
+returntoken&&token.length>0 ? token : undefined;
+}

lib/sandbox/__tests__/buildLifecycle.test.ts

@@ -0,0 +1,42 @@
+import{describe,it,expect}from"vitest";
+import{buildLifecycle}from"@/lib/sandbox/buildLifecycle";
+
+constISO="2030-01-01T00:00:00.000Z";
+constEPOCH=Date.parse(ISO);
+
+describe("buildLifecycle",()=>{
+it("converts every ISO timestamp on the row to epoch ms and sets serverTime",()=>{
+constrow={
+lifecycle_state: "active",
+last_activity_at: ISO,
+hibernate_after: ISO,
+sandbox_expires_at: ISO,
+}asany;
+
+constresult=buildLifecycle(row);
+
+expect(result).toEqual({
+serverTime: expect.any(Number),
+state: "active",
+lastActivityAt: EPOCH,
+hibernateAfter: EPOCH,
+sandboxExpiresAt: EPOCH,
+});
+});
+
+it("preserves null fields and a null lifecycle_state as-is",()=>{
+constrow={
+lifecycle_state: null,
+last_activity_at: null,
+hibernate_after: null,
+sandbox_expires_at: null,
+}asany;
+
+constresult=buildLifecycle(row);
+
+expect(result.state).toBeNull();
+expect(result.lastActivityAt).toBeNull();
+expect(result.hibernateAfter).toBeNull();
+expect(result.sandboxExpiresAt).toBeNull();
+});
+});

lib/sandbox/__tests__/buildSource.test.ts

@@ -0,0 +1,17 @@
+import{describe,it,expect}from"vitest";
+import{buildSource}from"@/lib/sandbox/buildSource";
+
+describe("buildSource",()=>{
+it("returns repo + branch when branch is provided",()=>{
+expect(buildSource({repoUrl: "https://github.com/o/r",branch: "main"})).toEqual({
+repo: "https://github.com/o/r",
+branch: "main",
+});
+});
+
+it("omits branch when not provided",()=>{
+expect(buildSource({repoUrl: "https://github.com/o/r"})).toEqual({
+repo: "https://github.com/o/r",
+});
+});
+});

lib/sandbox/__tests__/createSandboxHandler.test.ts

@@ -5,7 +5,7 @@ import { createSandboxHandler } from "@/lib/sandbox/createSandboxHandler";
 import{validateCreateSandboxBody}from"@/lib/sandbox/validateCreateSandboxBody";
 import{selectSessions}from"@/lib/supabase/sessions/selectSessions";
 import{connectSandbox}from"@/lib/sandbox/factory";
-import{updateSessionSandboxState}from"@/lib/supabase/sessions/updateSessionSandboxState";
+import{updateSession}from"@/lib/supabase/sessions/updateSession";
 
 vi.mock("@/lib/networking/getCorsHeaders",()=>({
 getCorsHeaders: ()=>({"Access-Control-Allow-Origin": "*"}),
@@ -19,8 +19,11 @@ vi.mock("@/lib/supabase/sessions/selectSessions", () => ({
 vi.mock("@/lib/sandbox/factory",()=>({
 connectSandbox: vi.fn(),
 }));
-vi.mock("@/lib/supabase/sessions/updateSessionSandboxState",()=>({
-updateSessionSandboxState: vi.fn(),
+vi.mock("@/lib/supabase/sessions/updateSession",()=>({
+updateSession: vi.fn(),
+}));
+vi.mock("@/lib/github/getServiceGithubToken",()=>({
+getServiceGithubToken: vi.fn(()=>"ghs_test_token"),
 }));
 
 constACCOUNT_ID="acc-1";
@@ -53,7 +56,7 @@ describe("createSandboxHandler", () => {
 vi.mocked(connectSandbox).mockResolvedValue(
 fakeSandbox()asunknownasAwaited<ReturnType<typeofconnectSandbox>>,
 );
-vi.mocked(updateSessionSandboxState).mockResolvedValue({}asany);
+vi.mocked(updateSession).mockResolvedValue({}asany);
 });
 
 it("short-circuits with the validator's response on validation failure",async()=>{
@@ -108,17 +111,29 @@ describe("createSandboxHandler", () => {
 expect(typeofbody.timing.readyMs).toBe("number");
 });
 
-it("persists sandbox state to the session row when sessionId is provided",async()=>{
+it("persists sandbox state and clears stale snapshot fields on the session row",async()=>{
 awaitcreateSandboxHandler(makeReq());
 
-expect(updateSessionSandboxState).toHaveBeenCalledWith(
+expect(updateSession).toHaveBeenCalledWith(
+"sess-1",
 expect.objectContaining({
-id: "sess-1",
-sandboxState: {type: "vercel",sandboxName: "session-sess-1"},
+sandbox_state: {type: "vercel",sandboxName: "session-sess-1"},
+lifecycle_state: "active",
+snapshot_url: null,
+snapshot_created_at: null,
 }),
 );
 });
 
+it("plumbs the service github token into connectSandbox options",async()=>{
+awaitcreateSandboxHandler(makeReq());
+
+constarg=vi.mocked(connectSandbox).mock.calls[0]?.[0];
+expect(arg).toBeDefined();
+if(!arg||!("options"inarg))thrownewError("expected new-API config shape");
+expect(arg.options?.githubToken).toBe("ghs_test_token");
+});
+
 it("skips the session-row write when no sessionId is provided",async()=>{
 vi.mocked(validateCreateSandboxBody).mockResolvedValueOnce({
 body: {repoUrl: "https://github.com/o/r",branch: "main"},
@@ -128,29 +143,7 @@ describe("createSandboxHandler", () => {
 constres=awaitcreateSandboxHandler(makeReq());
 
 expect(res.status).toBe(200);
-expect(updateSessionSandboxState).not.toHaveBeenCalled();
+expect(updateSession).not.toHaveBeenCalled();
 expect(selectSessions).not.toHaveBeenCalled();
 });
-
-it("uses isNewBranch=true to flip source.branch into source.newBranch",async()=>{
-vi.mocked(validateCreateSandboxBody).mockResolvedValueOnce({
-body: {
-repoUrl: "https://github.com/o/r",
-sessionId: "sess-1",
-branch: "feat/x",
-isNewBranch: true,
-},
-auth: {accountId: ACCOUNT_ID,orgId: null,authToken: "k"},
-});
-
-awaitcreateSandboxHandler(makeReq());
-
-constarg=vi.mocked(connectSandbox).mock.calls[0]?.[0];
-expect(arg).toBeDefined();
-if(!arg||!("state"inarg))thrownewError("expected new-API config shape");
-
-constsource=(arg.stateasany).source;
-expect(source.newBranch).toBe("feat/x");
-expect(source.branch).toBeUndefined();
-});
 });

lib/sandbox/__tests__/isSandboxActive.test.ts

@@ -0,0 +1,50 @@
+import{describe,it,expect}from"vitest";
+import{isSandboxActive}from"@/lib/sandbox/isSandboxActive";
+
+constFAR_FUTURE="2099-01-01T00:00:00.000Z";
+constFAR_PAST="2000-01-01T00:00:00.000Z";
+
+constbaseRow={
+sandbox_state: nullasunknown,
+sandbox_expires_at: nullasstring|null,
+};
+
+describe("isSandboxActive",()=>{
+it("returns false when sandbox_state has no runtime metadata",()=>{
+expect(isSandboxActive({ ...baseRow,sandbox_state: {type: "vercel"}}asany)).toBe(false);
+});
+
+it("returns false when sandbox_state is null",()=>{
+expect(isSandboxActive({ ...baseRow}asany)).toBe(false);
+});
+
+it("returns true with a runtime sandboxName and a far-future expiry",()=>{
+expect(
+isSandboxActive({
+ ...baseRow,
+sandbox_state: {type: "vercel",sandboxName: "session-x"},
+sandbox_expires_at: FAR_FUTURE,
+}asany),
+).toBe(true);
+});
+
+it("returns false when expiry is in the past",()=>{
+expect(
+isSandboxActive({
+ ...baseRow,
+sandbox_state: {type: "vercel",sandboxName: "session-x"},
+sandbox_expires_at: FAR_PAST,
+}asany),
+).toBe(false);
+});
+
+it("returns true when sandboxName is set but expiry is null (no expiry to compare against)",()=>{
+expect(
+isSandboxActive({
+ ...baseRow,
+sandbox_state: {type: "vercel",sandboxName: "session-x"},
+sandbox_expires_at: null,
+}asany),
+).toBe(true);
+});
+});

lib/sandbox/__tests__/isoToEpochMs.test.ts

@@ -0,0 +1,20 @@
+import{describe,it,expect}from"vitest";
+import{isoToEpochMs}from"@/lib/sandbox/isoToEpochMs";
+
+describe("isoToEpochMs",()=>{
+it("returns null for null input",()=>{
+expect(isoToEpochMs(null)).toBeNull();
+});
+
+it("returns null for an empty string",()=>{
+expect(isoToEpochMs("")).toBeNull();
+});
+
+it("returns null for an unparseable string",()=>{
+expect(isoToEpochMs("not-a-date")).toBeNull();
+});
+
+it("converts a valid ISO string to epoch milliseconds",()=>{
+expect(isoToEpochMs("2030-01-01T00:00:00.000Z")).toBe(Date.parse("2030-01-01T00:00:00.000Z"));
+});
+});

lib/sandbox/__tests__/validateCreateSandboxBody.test.ts

@@ -65,6 +65,20 @@ describe("validateCreateSandboxBody", () => {
 expect((resultasNextResponse).status).toBe(400);
 });
 
+it("returns 400 when repoUrl is not a valid GitHub repository URL",async()=>{
+constresult=awaitvalidateCreateSandboxBody(makeReq({repoUrl: "https://gitlab.com/o/r"}));
+
+expect(result).toBeInstanceOf(NextResponse);
+expect((resultasNextResponse).status).toBe(400);
+});
+
+it("returns 400 when repoUrl is not a URL at all",async()=>{
+constresult=awaitvalidateCreateSandboxBody(makeReq({repoUrl: "x"}));
+
+expect(result).toBeInstanceOf(NextResponse);
+expect((resultasNextResponse).status).toBe(400);
+});
+
 it("returns the validated body + auth on a minimal happy path",async()=>{
 constresult=awaitvalidateCreateSandboxBody(makeReq({repoUrl: "https://github.com/o/r"}));
 
@@ -73,31 +87,21 @@ describe("validateCreateSandboxBody", () => {
 expect(result.body.repoUrl).toBe("https://github.com/o/r");
 expect(result.body.sessionId).toBeUndefined();
 expect(result.body.branch).toBeUndefined();
-expect(result.body.isNewBranch).toBeUndefined();
 expect(result.auth.accountId).toBe(ACCOUNT_ID);
 });
 
-it("accepts a full request with sessionId, branch, isNewBranch",async()=>{
+it("accepts a full request with sessionId and branch",async()=>{
 constresult=awaitvalidateCreateSandboxBody(
 makeReq({
 repoUrl: "https://github.com/o/r",
 sessionId: "sess-1",
 branch: "feat/x",
-isNewBranch: true,
 }),
 );
 
 expect(result).not.toBeInstanceOf(NextResponse);
 if(resultinstanceofNextResponse)return;
 expect(result.body.sessionId).toBe("sess-1");
 expect(result.body.branch).toBe("feat/x");
-expect(result.body.isNewBranch).toBe(true);
-});
-
-it("returns 400 when isNewBranch is the wrong type",async()=>{
-constresult=awaitvalidateCreateSandboxBody(makeReq({repoUrl: "x",isNewBranch: "yes"}));
-
-expect(result).toBeInstanceOf(NextResponse);
-expect((resultasNextResponse).status).toBe(400);
 });
 });

lib/sandbox/buildLifecycle.ts

@@ -0,0 +1,19 @@
+import{isoToEpochMs}from"@/lib/sandbox/isoToEpochMs";
+importtype{Tables}from"@/types/database.types";
+
+/**
+ * Projects the lifecycle-relevant columns of a `sessions` row into the
+ * docs-spec lifecycle envelope used by GET /api/sandbox/status.
+ *
+ * @param row - The `sessions` row.
+ * @returns The lifecycle envelope: serverTime, state, and three epoch-ms timestamps.
+ */
+exportfunctionbuildLifecycle(row: Tables<"sessions">){
+return{
+serverTime: Date.now(),
+state: row.lifecycle_state,
+lastActivityAt: isoToEpochMs(row.last_activity_at),
+hibernateAfter: isoToEpochMs(row.hibernate_after),
+sandboxExpiresAt: isoToEpochMs(row.sandbox_expires_at),
+};
+}

lib/sandbox/buildSource.ts

@@ -0,0 +1,20 @@
+exportinterfaceSandboxSource{
+repo: string;
+branch?: string;
+}
+
+/**
+ * Builds the `source` shape that `connectSandbox` consumes.
+ *
+ * @param input - The repo URL and optional branch to check out.
+ * @returns A `{repo, branch?}` source descriptor.
+ */
+exportfunctionbuildSource({
+ repoUrl,
+ branch,
+}: {
+repoUrl: string;
+branch?: string;
+}): SandboxSource{
+returnbranch ? {repo: repoUrl, branch } : {repo: repoUrl};
+}