Recover server-v4 drizzle stack onto main#1917
Conversation
## Why Small cleanup / improvements that I wanted for adding drizzle cleanly - doing drizzle work in a separate PR for clarity - this PR is just the small improvements. ### What Changed - Use `t3-env` to parse env vars into a strongly typed object so the app can rely on defaults instead of carrying optional/null checks through the code. - Added a nested constants.ts to centralize shared constants and path segments in one place. - Run tests directly from source with `tsx` so we do not need compiled test output trees like `dist/tests/test`, which keeps the upcoming DB work cleaner. - Split `server.ts` into `app.ts` / `server.ts` split so Fastify app construction is cleanly testable and separate from process startup. This follows the official fastify demo best practice. - Replaced deprecated z.string().url() usages with z.url(). - Turned Fastify’s built-in logger on so local startup is visible by default (will be replaced by flowlogger - didn't implement any custom logger) ### Test plan - Added simple t3 env tests that check the discriminated union logic works <!-- This is an auto-generated description by cubic. --> --- ## Summary by cubic Make server-v4 runtime env type-safe and run node:test directly from source via `tsx`. Centralize Fastify app bootstrap and streamline the test harness for local/SEA/remote targets. - **Refactors** - Added `src/app.ts` to build the Fastify app, register OpenAPI, routes, health/readiness, and dev-only Swagger UI. - Introduced `src/env.ts` using `@t3-oss/env-core`; defaults `BROWSERBASE_CONFIG_DIR` to `~/.stagehand`, `PORT` to 3000, `NODE_ENV` to development; added unit tests. - Added `src/constants.ts` to centralize shared constants (e.g., default config dir name). - Switched URL fields to `z.url()` in LLM and Page schemas. - Reworked test runner to execute `.test.ts` from `packages/server-v4/test/**` with `--import=tsx`; simplified path handling and reporter names; removed dist test build and `tsconfig.tests.json`. - Simplified scripts: added `start`; updated test scripts to source paths; updated `turbo.json` to drop `build:esm-tests` and ensure integration depends on `build:server:dist`; kept SEA support. - Bumped `@t3-oss/env-core` to `^0.13.11`. - **Migration** - Start locally with `pnpm --filter @browserbasehq/stagehand-server-v4 start` (or `dev`). - Run tests from source: `pnpm test:unit` or `pnpm test:integration`. For SEA: `STAGEHAND_SERVER_TARGET=sea pnpm test:integration` (SEA build still required). - No more `build:esm-tests`; integration runs depend on `build:server:dist` automatically. <sup>Written for commit ebfdb12. Summary will update on new commits. <a href="https://cubic.dev/pr/browserbase/stagehand/pull/1896">Review in cubic</a></sup> <!-- End of auto-generated description by cubic. -->
Adds the Drizzle foundation for server-v4 with Postgres/PGlite runtime wiring and a Fastify DB plugin. This is DB-only scaffolding for the upcoming repo/service/controller work; no LLM persistence or execution logic is implemented yet. <!-- This is an auto-generated description by cubic. --> --- Add the Drizzle ORM foundation to `server-v4`, supporting both Postgres and local `pglite` via a Fastify database plugin with stricter env parsing and sane defaults. This wires up connections and migrations; schema tables and LLM logic will follow. - **New Features** - Fastify `databasePlugin` exposes `app.db`, `app.dbClient`, and `app.hasDatabase`. - Unified DB client for Postgres (`postgres`) and `pglite` with migrate/ping/close; auto-migrates on startup in `pglite` mode. - `STAGEHAND_DB_MODE` env selector (`pglite` default); `DATABASE_URL` must be a valid URL and is required only in Postgres mode; `BROWSERBASE_CONFIG_DIR` is normalized to an absolute path. - Drizzle config, empty schema barrel, and tests for env, DB client/plugin, and app boot/healthz. - **Migration** - Local dev: no setup needed. Defaults to `pglite`; data at `~/.stagehand/db/stagehand-v4`. - Postgres: set `STAGEHAND_DB_MODE=postgres` and `DATABASE_URL=<postgres-url>`, then run `pnpm run db:migrate` in `packages/server-v4`. - New scripts: `db:generate`, `db:migrate`, `db:push`, `db:studio`. - Note: no tables yet, so migrations are no-ops until the schema lands. <sup>Written for commit 6042c85. Summary will update on new commits. <a href="https://cubic.dev/pr/browserbase/stagehand/pull/1897">Review in cubic</a></sup> <!-- End of auto-generated description by cubic. -->
## Why Add the schema-first Drizzle layer in its own PR so the DB shape, generated Zod schemas, and first migration are easy to review before wiring in real repo/service persistence. ## What Changed - Added Drizzle table definitions for `llm_configs`, `llm_sessions`, `llm_calls`, `stagehand_browser_sessions`, and `stagehand_steps`. - Added a shared relations module and transaction-compatible DB executor types. - Added DB-derived Zod schemas from `drizzle-orm/zod` and made the v4/internal LLM schemas downstream of those DB schemas. - Switched public LLM ids to UUIDs so the API contract can flow directly from the DB schema. - Generated and committed the first Drizzle migration and updated the OpenAPI spec. - Added schema-focused tests to lock in the DB-derived Zod contract. ## Test Plan - `pnpm --filter @browserbasehq/stagehand-server-v4 run db:generate` - `pnpm --filter @browserbasehq/stagehand-server-v4 run db:migrate` - `pnpm --filter @browserbasehq/stagehand-server-v4 run lint` - `pnpm --filter @browserbasehq/stagehand-server-v4 run test:unit` - `pnpm --filter @browserbasehq/stagehand-server-v4 run build` - `pnpm --filter @browserbasehq/stagehand-server-v4 run gen:openapi` - `CHROME_PATH=/bin/echo STAGEHAND_SERVER_TARGET=local pnpm --filter @browserbasehq/stagehand-server-v4 run test:server -- packages/server-v4/test/integration/v4/llms.test.ts packages/server-v4/test/integration/v4/browsersession.test.ts` <!-- This is an auto-generated description by cubic. --> --- ## Summary by cubic Adds a schema-first `drizzle-orm` layer and wires `/v4/llms` to a DB-backed controller/service/repo. Browser session routes now validate LLM IDs and auto-create a default LLM when none is provided. - **New Features** - Added Drizzle tables and relations for LLMs and Stagehand; generated Zod schemas from the DB; typed executors and transaction types for Postgres and `pglite`. - Implemented LLM config persistence with `llmModulePlugin`, controller/service/repo; `/v4/llms` list/get/create/update now use the DB. - Browser session create/update resolves LLM IDs via the service and creates a system default LLM if `llmId` is missing (uses constants for name/model). - Updated OpenAPI: UUID `LLMId` with strict format, nullable fields modeled, timestamps as strings; public create/update derive from DB schemas and exclude id/source. - Added the first migration and schema-focused unit tests. - **Migration** - Run `db:migrate` in `packages/server-v4`. - Update clients to treat `LLMId` as a UUID. - Regenerate API clients from `openapi.v4.yaml` if needed. <sup>Written for commit f69ef51. Summary will update on new commits. <a href="https://cubic.dev/pr/browserbase/stagehand/pull/1898">Review in cubic</a></sup> <!-- End of auto-generated description by cubic. -->
|
There was a problem hiding this comment.
6 issues found across 44 files
Confidence score: 3/5
- There is moderate merge risk because multiple medium-severity (6/10) issues have concrete runtime impact:
packages/server-v4/src/db/plugin.tscan leak an open DB client on startup-check failure, andpackages/server-v4/src/routes/v4/browsersession/index.tscan create orphansystem-defaultrows when secondary LLM validation fails. turbo.jsonintroduces regression risk in CI/dev workflows sincetest:serverno longer guaranteesdist/testsis built for server-v3 scripts; adding thebuild:esm-testsdependency should stabilize test execution.- Lower-severity items in
packages/server-v4/test/unit/db/databaseClient.test.tsand the 404 schema mismatch inpackages/server-v4/src/routes/v4/browsersession/index.tsare cleanup/contract issues, but they still add noise and can mask failures if left unresolved. - Pay close attention to
packages/server-v4/src/db/plugin.ts,packages/server-v4/src/routes/v4/browsersession/index.ts, andturbo.json- they carry the main resource-leak, data-integrity, and test-pipeline regression risks.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/server-v4/src/db/plugin.ts">
<violation number="1" location="packages/server-v4/src/db/plugin.ts:18">
P2: Close the DB connection when startup checks fail. Right now a migrate/ping error can leave an opened DB client because cleanup is only registered after those calls succeed.</violation>
</file>
<file name="packages/server-v4/test/unit/db/databaseClient.test.ts">
<violation number="1" location="packages/server-v4/test/unit/db/databaseClient.test.ts:19">
P3: Prefer forward-slash path construction instead of `path.join` here so generated paths stay normalized across platforms.
(Based on your team's feedback about using forward slashes instead of `path.join` for path construction.) [FEEDBACK_USED]</violation>
<violation number="2" location="packages/server-v4/test/unit/db/databaseClient.test.ts:23">
P2: Close each database connection in a `finally` block so cleanup runs even when setup/query/assertion fails.</violation>
</file>
<file name="packages/server-v4/src/routes/v4/browsersession/index.ts">
<violation number="1" location="packages/server-v4/src/routes/v4/browsersession/index.ts:19">
P2: Default LLM creation happens before secondary LLM ID validation, so failed requests can still insert orphan `system-default` rows.</violation>
<violation number="2" location="packages/server-v4/src/routes/v4/browsersession/index.ts:21">
P3: `getLlm` adds a reachable 404 path, but the route schema does not declare a 404 response.</violation>
</file>
<file name="turbo.json">
<violation number="1" location="turbo.json:502">
P2: `test:server` no longer builds the compiled `dist/tests` that server-v3 test scripts rely on. Add the `build:esm-tests` dependency so the tests exist before running.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Client
participant App as Fastify Server
participant DBPlugin as Database Plugin
participant Svc as LLM Service
participant Repo as LLM Repository
participant Drz as Drizzle ORM
participant DB as Postgres / PGlite
Note over App, DB: NEW: Server-v4 Startup & Dependency Injection
App->>DBPlugin: Register Plugin
DBPlugin->>DBPlugin: NEW: Parse STAGEHAND_DB_MODE
alt mode == 'pglite'
DBPlugin->>DB: Open local storage (~/.stagehand/db)
DBPlugin->>Drz: NEW: Run Migrations (migrateOnStartup)
else mode == 'postgres'
DBPlugin->>DB: Connect to DATABASE_URL
end
DBPlugin-->>App: Decorate app.db & app.dbClient
Note over Client, DB: NEW: DB-Backed LLM Configuration Flow
Client->>App: POST /v4/llms (LLMCreateRequest)
App->>Svc: createLlm(input)
Svc->>Svc: Validate via Zod (llmConfigInsertSchema)
Svc->>Repo: create(values)
Repo->>Drz: insert(llmConfigs)
Drz->>DB: SQL INSERT
DB-->>Repo: New LLM Record
Repo-->>Svc: LLMConfigSelect
Svc-->>App: LLM Result
App-->>Client: 200 OK + LLM JSON
Note over Client, DB: CHANGED: Browser Session Creation with LLM Reference
Client->>App: POST /v4/browsersession (env, llmId?)
alt NEW: llmId provided
App->>Svc: getLlm(id)
Svc->>Repo: getById(id)
Repo-->>App: LLM Config
else NEW: llmId missing
App->>Svc: createSystemDefaultLlm()
Svc->>Repo: create(source: 'system-default')
Repo-->>App: New Default LLM
end
App->>App: CHANGED: Create session in stubState with UUID LLMId
App-->>Client: 200 OK + BrowserSession (UUID id)
Note over Client, DB: Error Handling & Validation
Client->>App: GET /v4/llms/:id (Invalid UUID)
App->>App: NEW: Zod Validation (LLMIdParamsSchema)
App-->>Client: 400 Bad Request (Validation failed)
Client->>App: GET /v4/llms/:id (Valid UUID, missing record)
App->>Svc: getLlm(id)
Svc->>Repo: getById(id)
Repo-->>Svc: null
Svc-->>App: Throw 404 (notFoundError)
App-->>Client: 404 Not Found (LLM not found)
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| app.decorate("dbClient", null); | ||
| app.decorate("hasDatabase", false); | ||
|
|
||
| const connection = await createDatabaseConnection(database); |
There was a problem hiding this comment.
P2: Close the DB connection when startup checks fail. Right now a migrate/ping error can leave an opened DB client because cleanup is only registered after those calls succeed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/server-v4/src/db/plugin.ts, line 18:
<comment>Close the DB connection when startup checks fail. Right now a migrate/ping error can leave an opened DB client because cleanup is only registered after those calls succeed.</comment>
<file context>
@@ -0,0 +1,36 @@
+ app.decorate("dbClient", null);
+ app.decorate("hasDatabase", false);
+
+ const connection = await createDatabaseConnection(database);
+
+ if (migrateOnStartup) {
</file context>
| const llm = | ||
| body.llmId !== undefined | ||
| ? await request.server.llmService.getLlm(body.llmId) | ||
| : await request.server.llmService.createSystemDefaultLlm(); | ||
| const [actLlmId, observeLlmId, extractLlmId] = await Promise.all([ | ||
| body.actLlmId | ||
| ? request.server.llmService | ||
| .getLlm(body.actLlmId) | ||
| .then((value) => value.id) | ||
| : Promise.resolve(null), | ||
| body.observeLlmId | ||
| ? request.server.llmService | ||
| .getLlm(body.observeLlmId) | ||
| .then((value) => value.id) | ||
| : Promise.resolve(null), | ||
| body.extractLlmId | ||
| ? request.server.llmService | ||
| .getLlm(body.extractLlmId) | ||
| .then((value) => value.id) | ||
| : Promise.resolve(null), | ||
| ]); |
There was a problem hiding this comment.
P2: Default LLM creation happens before secondary LLM ID validation, so failed requests can still insert orphan system-default rows.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/server-v4/src/routes/v4/browsersession/index.ts, line 19:
<comment>Default LLM creation happens before secondary LLM ID validation, so failed requests can still insert orphan `system-default` rows.</comment>
<file context>
@@ -16,7 +16,34 @@ const createBrowserSessionHandler: RouteHandlerMethod = async (
) => {
const body = request.body as BrowserSessionCreateRequest;
- const browserSession = createBrowserSession(body);
+ const llm =
+ body.llmId !== undefined
+ ? await request.server.llmService.getLlm(body.llmId)
</file context>
| const llm = | |
| body.llmId !== undefined | |
| ? await request.server.llmService.getLlm(body.llmId) | |
| : await request.server.llmService.createSystemDefaultLlm(); | |
| const [actLlmId, observeLlmId, extractLlmId] = await Promise.all([ | |
| body.actLlmId | |
| ? request.server.llmService | |
| .getLlm(body.actLlmId) | |
| .then((value) => value.id) | |
| : Promise.resolve(null), | |
| body.observeLlmId | |
| ? request.server.llmService | |
| .getLlm(body.observeLlmId) | |
| .then((value) => value.id) | |
| : Promise.resolve(null), | |
| body.extractLlmId | |
| ? request.server.llmService | |
| .getLlm(body.extractLlmId) | |
| .then((value) => value.id) | |
| : Promise.resolve(null), | |
| ]); | |
| const [actLlmId, observeLlmId, extractLlmId] = await Promise.all([ | |
| body.actLlmId | |
| ? request.server.llmService | |
| .getLlm(body.actLlmId) | |
| .then((value) => value.id) | |
| : Promise.resolve(null), | |
| body.observeLlmId | |
| ? request.server.llmService | |
| .getLlm(body.observeLlmId) | |
| .then((value) => value.id) | |
| : Promise.resolve(null), | |
| body.extractLlmId | |
| ? request.server.llmService | |
| .getLlm(body.extractLlmId) | |
| .then((value) => value.id) | |
| : Promise.resolve(null), | |
| ]); | |
| const llm = | |
| body.llmId !== undefined | |
| ? await request.server.llmService.getLlm(body.llmId) | |
| : await request.server.llmService.createSystemDefaultLlm(); |
There was a problem hiding this comment.
2 issues found across 8 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="turbo.json">
<violation number="1" location="turbo.json:73">
P1: `build:esm-tests` outputs are under-declared. The task emits `dist/src/**` as well, so caching/restores can drop required files and break dist-based test execution.</violation>
<violation number="2" location="turbo.json:75">
P1: `build:esm-tests` cache inputs are incomplete: it compiles `src/**/*.ts` but only tracks `tests/**/*.ts`. Add source files to inputs so Turbo invalidates this task when server-v3 source changes.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
Why
The original server-v4 stacked PRs merged into each other after
STG-1614had already merged intomain, so the env/test-harness work, Drizzle foundation, and Drizzle schema/controller flow never actually reachedmain.This PR recovers that full stack onto current
mainin one branch.What Changed
#1896,#1897, and#1898onto freshmainserver-v4env/test harness cleanup, Drizzle foundation, DB smoke tests, schema/migration work, and/v4/llmscontroller/service/repo flowpnpm-lock.yamlagainst currentmainafter applying the recovered stackTest Plan
pnpm installpnpm --filter @browserbasehq/stagehand-server-v4 run lintpnpm --filter @browserbasehq/stagehand-server-v4 run test:unitpnpm --filter @browserbasehq/stagehand-server-v4 run buildpnpm --filter @browserbasehq/stagehand-server-v4 run gen:openapiSummary by cubic
Recovers the server‑v4 Drizzle stack onto
main, restoring DB‑backed/v4/llmsand switching the publicLLMIdto UUID. Adds a typed env, clean app bootstrap, and a localpglite/Postgres runtime with migrations, tests, and a simpler source‑based test harness.New Features
pglitedefault and Postgres option; newdb:*scripts viadrizzle-kit.databasePluginand unified DB client;app.tsbootstrap with health/readiness and Swagger/OpenAPI; env via@t3-oss/env-core.LLMIdwith strict format and pattern./v4/llms; routes now delegate to the controller; browser session create uses the service and auto‑creates a system default LLM whenllmIdis missing.tsx; test server script simplified;turbotasks cleaned up; schema cleanup (z.url()).Migration
pnpm --filter @browserbasehq/stagehand-server-v4 db:migrate.pgliteat~/.stagehand/db/stagehand-v4. For Postgres setSTAGEHAND_DB_MODE=postgresandDATABASE_URL.LLMIdas a UUID and regenerate fromopenapi.v4.yamlif needed.Written for commit 1fb861e. Summary will update on new commits. Review in cubic