diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index afe4561b4..2ef665f03 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -68,6 +68,12 @@ jobs: - run: | mkdir -p "$RUNNER_TEMP/hub-data" echo "HUB_DATA_DIR=$RUNNER_TEMP/hub-data" >> "$GITHUB_ENV" + # Browser/mcp suites set DATABASE_URL and go through dbGate, which + # redirects the resolved url onto the derived _e2e sibling (see e2e/lib/ + # database-url.ts). Provision + migrate that sibling here (idempotent, + # CREATE IF NOT EXISTS + migrateHub) so the browser shards do not fail on + # 'database workbench_e2e does not exist'. + - run: bun scripts/db-setup.ts - run: | LC_ALL=C i=0 @@ -119,4 +125,10 @@ jobs: - run: bun run build working-directory: packages/worker - run: bun test e2e/lib/web-build.test.ts + # Provision + migrate both the base database AND the derived _e2e sibling. + # scripts/db-setup.ts (DATABASE_URL is already set for this job) creates + # and runs migrateHub against DATABASE_URL's db and its _e2e sibling, so + # direct-DB suites (mailbox persist/send) are not order-dependent on some + # hub-booting suite migrating first. + - run: bun scripts/db-setup.ts - run: bun test e2e/grants e2e/hub e2e/install e2e/workflows diff --git a/apps/web/src/onboarding/provider-connect-step.tsx b/apps/web/src/onboarding/provider-connect-step.tsx index 4d7f45e5d..dcc100382 100644 --- a/apps/web/src/onboarding/provider-connect-step.tsx +++ b/apps/web/src/onboarding/provider-connect-step.tsx @@ -259,7 +259,7 @@ export function ProviderConnectStep({ : option.canonicalName; const resolvedLabel = models.find((model) => model.id === resolvedModel)?.label ?? resolvedModel; const urlCheck = baseUrlSchema(baseURL); - const urlError = customURL && baseURL.trim() !== "" && urlCheck instanceof type.errors; + const urlError = customURL && (baseURL.trim() === "" || urlCheck instanceof type.errors); // Fetched straight from the browser to the user-supplied base URL — this // never touches the hub. Validates the base URL is actually an Ollama diff --git a/apps/web/src/settings/api-request.ts b/apps/web/src/settings/api-request.ts index 3c50d667c..c3e77948a 100644 --- a/apps/web/src/settings/api-request.ts +++ b/apps/web/src/settings/api-request.ts @@ -42,7 +42,6 @@ export async function apiRequest( const body: unknown = await response.json().catch(() => undefined); throw new ErrorCtor(readErrorEnvelope(response.status, body, verb), response.status); } - if (response.status === 204) return undefined as T; const body: unknown = await response.json().catch(() => undefined); const parsed = schema(body); if (parsed instanceof type.errors) { @@ -50,3 +49,38 @@ export async function apiRequest( } return parsed; } + +/** For a route whose success response is `204 No Content` — nothing to + * validate against a schema, so this is the one place a call is allowed + * to end without a parsed body, rather than every `request` caller + * having to accept an unsound `undefined as T`. */ +export async function requestVoid( + path: string, + verb: string, + ErrorCtor: ApiErrorCtor, + init?: RequestInit, +): Promise { + let response: Response; + try { + response = await fetch(path, { + ...init, + headers: { "content-type": "application/json", ...init?.headers }, + }); + } catch (cause) { + throw new ErrorCtor(cause instanceof Error ? cause.message : String(cause)); + } + if (!response.ok) { + const body: unknown = await response.json().catch(() => undefined); + throw new ErrorCtor(readErrorEnvelope(response.status, body, verb), response.status); + } + // A void route answers `204 No Content` by contract. Any other 2xx (e.g. + // a 200/201-with-body a route drifted into returning where 204 is expected) + // is a contract mismatch and must fail loudly rather than be silently + // accepted as an empty success. + if (response.status !== 204) { + throw new ErrorCtor( + `Unexpected response while ${verb}: expected 204 No Content, got ${response.status}`, + response.status, + ); + } +} diff --git a/apps/web/src/settings/credentials-api.ts b/apps/web/src/settings/credentials-api.ts index 8915f854b..326d2adde 100644 --- a/apps/web/src/settings/credentials-api.ts +++ b/apps/web/src/settings/credentials-api.ts @@ -10,7 +10,7 @@ import { } from "@intx/types"; export { credentialTypes } from "@intx/types"; -import { apiRequest, type Validator } from "./api-request"; +import { apiRequest, requestVoid, type Validator } from "./api-request"; export type Credential = typeof CredentialResponse.infer; export type Provider = typeof ProviderResponse.infer; @@ -80,10 +80,10 @@ export function createCredential( } export function deleteCredential(tenantId: string, credentialId: string): Promise { - return request( + return requestVoid( `/api/tenants/${tenantId}/credentials/${credentialId}`, - (data) => data as void, "revoking that credential", + CredentialsApiError, { method: "DELETE" }, ); } diff --git a/apps/web/src/settings/inference/api.test.ts b/apps/web/src/settings/inference/api.test.ts new file mode 100644 index 000000000..917b65065 --- /dev/null +++ b/apps/web/src/settings/inference/api.test.ts @@ -0,0 +1,102 @@ +// Regression coverage for the inference/api `requestVoid` seam (exercised +// through `deleteOwnOffering`, the one database-less void route the Inference +// section calls, which also proves the `fetchImpl` seam is honored): a literal +// 204 must be the only 2xx success, any other 2xx is a contract drift that +// fails loudly, and the cardinal error envelope is surfaced with its status. +import { beforeEach, describe, expect, test } from "bun:test"; + +import { deleteOwnOffering, InferenceSettingsApiError } from "./api"; + +const TENANT = "tenant-1"; +const OFFERING = "offering-1"; + +type StubCall = { readonly input: string | URL | Request; readonly init: RequestInit | undefined }; +const calls: StubCall[] = []; + +function stubFetch(status: number, body?: unknown): typeof fetch { + return (async (input: string | URL | Request, init?: RequestInit) => { + calls.push({ input, init }); + return new Response(body === undefined ? "" : JSON.stringify(body), { + status, + headers: { "content-type": "application/json" }, + }); + }) as typeof fetch; +} + +beforeEach(() => { + calls.length = 0; +}); + +describe("inference/api deleteOwnOffering (requestVoid)", () => { + test("a literal 204 resolves", async () => { + await expect(deleteOwnOffering(TENANT, OFFERING, stubFetch(204))).resolves.toBeUndefined(); + expect(calls).toHaveLength(1); + expect(calls[0]!.init?.method).toBe("DELETE"); + }); + + test("a 2xx other than 204 is a contract drift and fails loudly", async () => { + const error = await deleteOwnOffering(TENANT, OFFERING, stubFetch(200, { ok: true })).catch( + (cause: unknown) => cause, + ); + expect(error).toBeInstanceOf(InferenceSettingsApiError); + expect((error as InferenceSettingsApiError).status).toBe(200); + expect((error as InferenceSettingsApiError).message).toContain("204"); + }); + + test("a 201 is likewise rejected", async () => { + const error = await deleteOwnOffering( + TENANT, + OFFERING, + stubFetch(201, { created: true }), + ).catch((cause: unknown) => cause); + expect(error).toBeInstanceOf(InferenceSettingsApiError); + expect((error as InferenceSettingsApiError).status).toBe(201); + }); + + test("a 401 surfaces the envelope's userMessage with the status", async () => { + const error = await deleteOwnOffering( + TENANT, + OFFERING, + stubFetch(401, { + error: { code: "unauthorized", userMessage: "Sign in again", refId: "r" }, + }), + ).catch((cause: unknown) => cause); + expect(error).toBeInstanceOf(InferenceSettingsApiError); + expect((error as InferenceSettingsApiError).status).toBe(401); + expect((error as InferenceSettingsApiError).message).toBe("Sign in again"); + }); + + test("a 403 surfaces the envelope's userMessage with the status", async () => { + const error = await deleteOwnOffering( + TENANT, + OFFERING, + stubFetch(403, { + error: { code: "forbidden", userMessage: "Not permitted", refId: "r" }, + }), + ).catch((cause: unknown) => cause); + expect(error).toBeInstanceOf(InferenceSettingsApiError); + expect((error as InferenceSettingsApiError).status).toBe(403); + expect((error as InferenceSettingsApiError).message).toBe("Not permitted"); + }); + + test("a 5xx falls back to a generic path-free sentence", async () => { + const error = await deleteOwnOffering( + TENANT, + OFFERING, + stubFetch(500, { notAnEnvelope: true }), + ).catch((cause: unknown) => cause); + expect(error).toBeInstanceOf(InferenceSettingsApiError); + expect((error as InferenceSettingsApiError).status).toBe(500); + }); + + test("a network failure maps to an InferenceSettingsApiError", async () => { + const throwing = (async () => { + throw new TypeError("fetch failed"); + }) as unknown as typeof fetch; + const error = await deleteOwnOffering(TENANT, OFFERING, throwing).catch( + (cause: unknown) => cause, + ); + expect(error).toBeInstanceOf(InferenceSettingsApiError); + expect((error as InferenceSettingsApiError).status).toBeUndefined(); + }); +}); diff --git a/apps/web/src/settings/inference/api.ts b/apps/web/src/settings/inference/api.ts index 8a76fcf89..534219098 100644 --- a/apps/web/src/settings/inference/api.ts +++ b/apps/web/src/settings/inference/api.ts @@ -30,6 +30,7 @@ // reference it — never the ancestor's credential. import { type } from "arktype"; +import { reportError } from "@corbits/error-sink"; import { CreateCredential, CreateModel, @@ -146,6 +147,15 @@ async function requestVoid( response.status, ); } + // A void route answers `204 No Content` by contract. Any other 2xx (e.g. a + // 200/201-with-body the route drifted into returning where 204 is expected) + // is a contract mismatch and must fail loudly. + if (response.status !== 204) { + throw new InferenceSettingsApiError( + `The server answered ${response.status} while ${verb}; expected 204 No Content.`, + response.status, + ); + } } /** The tenant's resolved catalog — every model visible after inheritance, @@ -365,7 +375,9 @@ export async function shadowOffering( "rolling back the just-minted provider", { method: "DELETE" }, fetchImpl, - ).catch(() => undefined); + ).catch((rollbackCause: unknown) => { + reportError(rollbackCause, { operation: "shadowOffering.rollback", tenantId }); + }); } throw cause; } diff --git a/apps/web/src/settings/tenancy-api.test.ts b/apps/web/src/settings/tenancy-api.test.ts new file mode 100644 index 000000000..acceed8e6 --- /dev/null +++ b/apps/web/src/settings/tenancy-api.test.ts @@ -0,0 +1,84 @@ +// Regression coverage for the tenancy-api `requestVoid` seam (exercised +// through `removePrincipal`, which is the one delete route the People panel +// always hits): a literal 204 must be the only 2xx success, any other 2xx is +// a contract drift that fails loudly, and 401/403 map to their distinct errors +// (UnauthenticatedError / TenancyApiError-with-status) before the generic +// non-ok fallback runs. +import { afterEach, describe, expect, test } from "bun:test"; + +import { UnauthenticatedError } from "@/lib/api-query"; +import { removePrincipal, TenancyApiError } from "./tenancy-api"; + +const TENANT = "tenant-1"; +const PRINCIPAL = "prn-1"; + +function stubFetch(status: number, body?: unknown): typeof fetch { + return (async (_input: string | URL | Request, _init?: RequestInit) => { + return new Response(body === undefined ? "" : JSON.stringify(body), { + status, + headers: { "content-type": "application/json" }, + }); + }) as typeof fetch; +} + +const originalFetch = globalThis.fetch; +afterEach(() => { + globalThis.fetch = originalFetch; +}); + +describe("tenancy-api removePrincipal (requestVoid)", () => { + test("a literal 204 resolves", async () => { + globalThis.fetch = stubFetch(204); + await expect(removePrincipal(TENANT, PRINCIPAL)).resolves.toBeUndefined(); + }); + + test("a 2xx other than 204 is a contract drift and fails loudly", async () => { + globalThis.fetch = stubFetch(200, { ok: true }); + const error = await removePrincipal(TENANT, PRINCIPAL).catch((cause: unknown) => cause); + expect(error).toBeInstanceOf(TenancyApiError); + expect((error as TenancyApiError).status).toBe(200); + expect((error as TenancyApiError).message).toContain("204"); + }); + + test("a 201 is likewise rejected", async () => { + globalThis.fetch = stubFetch(201, { created: true }); + const error = await removePrincipal(TENANT, PRINCIPAL).catch((cause: unknown) => cause); + expect(error).toBeInstanceOf(TenancyApiError); + expect((error as TenancyApiError).status).toBe(201); + }); + + test("a 401 maps to UnauthenticatedError", async () => { + globalThis.fetch = stubFetch(401, { + error: { code: "unauthorized", userMessage: "Sign in again", refId: "r" }, + }); + const error = await removePrincipal(TENANT, PRINCIPAL).catch((cause: unknown) => cause); + expect(error).toBeInstanceOf(UnauthenticatedError); + }); + + test("a 403 maps to a statused TenancyApiError", async () => { + globalThis.fetch = stubFetch(403, { + error: { code: "forbidden", userMessage: "Not permitted", refId: "r" }, + }); + const error = await removePrincipal(TENANT, PRINCIPAL).catch((cause: unknown) => cause); + expect(error).toBeInstanceOf(TenancyApiError); + expect((error as TenancyApiError).status).toBe(403); + }); + + test("a generic 5xx still lands on the statused TenancyApiError", async () => { + globalThis.fetch = stubFetch(500, { + error: { code: "boom", userMessage: "Something broke", refId: "r" }, + }); + const error = await removePrincipal(TENANT, PRINCIPAL).catch((cause: unknown) => cause); + expect(error).toBeInstanceOf(TenancyApiError); + expect((error as TenancyApiError).status).toBe(500); + }); + + test("a network failure maps to a TenancyApiError with no status", async () => { + globalThis.fetch = (async () => { + throw new TypeError("fetch failed"); + }) as unknown as typeof fetch; + const error = await removePrincipal(TENANT, PRINCIPAL).catch((cause: unknown) => cause); + expect(error).toBeInstanceOf(TenancyApiError); + expect((error as TenancyApiError).status).toBeUndefined(); + }); +}); diff --git a/apps/web/src/settings/tenancy-api.ts b/apps/web/src/settings/tenancy-api.ts index 7122f6d93..a0920fb3d 100644 --- a/apps/web/src/settings/tenancy-api.ts +++ b/apps/web/src/settings/tenancy-api.ts @@ -62,7 +62,6 @@ async function request(path: string, schema: Validator, init?: RequestInit if (!response.ok) { throw new TenancyApiError(`The hub answered ${response.status} for ${path}.`, response.status); } - if (response.status === 204) return undefined as T; const body: unknown = await response.json().catch(() => undefined); const parsed = schema(body); if (parsed instanceof type.errors) { @@ -71,6 +70,40 @@ async function request(path: string, schema: Validator, init?: RequestInit return parsed; } +/** For a route whose success response is `204 No Content` — nothing to + * validate against a schema, so this is the one place a call is allowed + * to end without a parsed body, rather than every `request` caller + * having to accept an unsound `undefined as T`. */ +async function requestVoid(path: string, init?: RequestInit): Promise { + let response: Response; + try { + response = await fetch(path, { + ...init, + headers: { "content-type": "application/json", ...init?.headers }, + }); + } catch (cause) { + throw new TenancyApiError(cause instanceof Error ? cause.message : String(cause)); + } + if (response.status === 401) { + throw new UnauthenticatedError(); + } + if (response.status === 403) { + throw new TenancyApiError(`Not permitted to view ${path}.`, 403); + } + if (!response.ok) { + throw new TenancyApiError(`The hub answered ${response.status} for ${path}.`, response.status); + } + // A void route answers `204 No Content` by contract. Any other 2xx (e.g. a + // 200/201-with-body the route drifted into returning where 204 is expected) + // is a contract mismatch and must fail loudly. + if (response.status !== 204) { + throw new TenancyApiError( + `The hub answered ${response.status} for ${path}; expected 204 No Content.`, + response.status, + ); + } +} + // -- Principals -------------------------------------------------------- export function listPrincipals(tenantId: string): Promise { @@ -89,11 +122,7 @@ export function updatePrincipalStatus( } export function removePrincipal(tenantId: string, principalId: string): Promise { - return request( - `/api/tenants/${tenantId}/principals/${principalId}`, - (data) => data as void, - { method: "DELETE" }, - ); + return requestVoid(`/api/tenants/${tenantId}/principals/${principalId}`, { method: "DELETE" }); } export type InviteMemberInput = typeof InviteMember.infer; @@ -135,25 +164,19 @@ export function renameRole( } export function deleteRole(tenantId: string, roleId: string): Promise { - return request(`/api/tenants/${tenantId}/roles/${roleId}`, (data) => data as void, { - method: "DELETE", - }); + return requestVoid(`/api/tenants/${tenantId}/roles/${roleId}`, { method: "DELETE" }); } export function assignRole(tenantId: string, principalId: string, roleId: string): Promise { - return request( - `/api/tenants/${tenantId}/principals/${principalId}/roles/${roleId}`, - (data) => data as void, - { method: "POST" }, - ); + return requestVoid(`/api/tenants/${tenantId}/principals/${principalId}/roles/${roleId}`, { + method: "POST", + }); } export function unassignRole(tenantId: string, principalId: string, roleId: string): Promise { - return request( - `/api/tenants/${tenantId}/principals/${principalId}/roles/${roleId}`, - (data) => data as void, - { method: "DELETE" }, - ); + return requestVoid(`/api/tenants/${tenantId}/principals/${principalId}/roles/${roleId}`, { + method: "DELETE", + }); } // -- Grants ---------------------------------------------------------------- @@ -213,9 +236,7 @@ export function updateGrant( } export function revokeGrant(tenantId: string, grantId: string): Promise { - return request(`/api/tenants/${tenantId}/grants/${grantId}`, (data) => data as void, { - method: "DELETE", - }); + return requestVoid(`/api/tenants/${tenantId}/grants/${grantId}`, { method: "DELETE" }); } // The one grant-checked route that itself requires no grant, so it diff --git a/bun.lock b/bun.lock index 2d3f0dde7..95e9d8135 100644 --- a/bun.lock +++ b/bun.lock @@ -155,6 +155,9 @@ "packages/approvals": { "name": "@corbits/approvals", "version": "0.0.1", + "dependencies": { + "@corbits/error-sink": "workspace:*", + }, "devDependencies": { "@types/bun": "catalog:", "typescript": "catalog:", @@ -1775,16 +1778,10 @@ "@corbits/mailbox/hono-openapi": ["hono-openapi@1.3.1", "", { "peerDependencies": { "@hono/standard-validator": "^0.2.0", "@standard-community/standard-json": "^0.3.5", "@standard-community/standard-openapi": "^0.2.9", "@types/json-schema": "^7.0.15", "hono": "^4.11.2", "openapi-types": "^12.1.3" }, "optionalPeers": ["@hono/standard-validator", "hono"] }, "sha512-NLVeVkhKZ3drmQNEIPac8HX8Y54uf1hJAgIM/7MfDsaeVVmB+QILWQxx5x3R3NvRHgedcbEbOCGY2uR7WQYyMw=="], - "@corbits/worker/@corbits/artifacts": ["@corbits/artifacts@github:corbitsdev/corbits-artifacts#4a2a3c2", { "dependencies": { "@hono/standard-validator": "^0.2.3" }, "peerDependencies": { "@intx/agent": "^0.3.0", "@intx/hub-api": "^0.3.0", "@intx/types": "^0.3.0", "arktype": "^2.1.29", "drizzle-orm": "^0.45.2", "hono": "^4.12.32", "hono-openapi": "^1.2.0", "postgres": "^3.4.9" } }, "corbitsdev-corbits-artifacts-4a2a3c2", "sha512-UY5nkQ31kGmYgqgIOSlbigwvm7c/s2kpqEbF6Oqo0rzoS1RejpPp+w/Yim6h2MRpYCl5CXgAET1MhkDboHuWBA=="], - "@esbuild-kit/core-utils/esbuild": ["esbuild@0.18.20", "", { "optionalDependencies": { "@esbuild/android-arm": "0.18.20", "@esbuild/android-arm64": "0.18.20", "@esbuild/android-x64": "0.18.20", "@esbuild/darwin-arm64": "0.18.20", "@esbuild/darwin-x64": "0.18.20", "@esbuild/freebsd-arm64": "0.18.20", "@esbuild/freebsd-x64": "0.18.20", "@esbuild/linux-arm": "0.18.20", "@esbuild/linux-arm64": "0.18.20", "@esbuild/linux-ia32": "0.18.20", "@esbuild/linux-loong64": "0.18.20", "@esbuild/linux-mips64el": "0.18.20", "@esbuild/linux-ppc64": "0.18.20", "@esbuild/linux-riscv64": "0.18.20", "@esbuild/linux-s390x": "0.18.20", "@esbuild/linux-x64": "0.18.20", "@esbuild/netbsd-x64": "0.18.20", "@esbuild/openbsd-x64": "0.18.20", "@esbuild/sunos-x64": "0.18.20", "@esbuild/win32-arm64": "0.18.20", "@esbuild/win32-ia32": "0.18.20", "@esbuild/win32-x64": "0.18.20" }, "bin": { "esbuild": "bin/esbuild" } }, "sha512-ceqxoedUrcayh7Y7ZX6NdbbDzGROiyVBgC4PriJThBKSVPWnnFHZAkfI1lJT8QFkOwH4qOS2SJkS4wvpGl8BpA=="], "@npmcli/agent/lru-cache": ["lru-cache@11.5.3", "", {}, "sha512-U4N8FgzmWxc8k1VH8Kr6lQg18U7Fjvby6wXHVRX/ZZ7IwWbRMgrRbP0Wrb5q5NVinryp4SQampHKdvtecItxUg=="], - "@workbench/hub/@corbits/artifacts": ["@corbits/artifacts@github:corbitsdev/corbits-artifacts#4a2a3c2", { "dependencies": { "@hono/standard-validator": "^0.2.3" }, "peerDependencies": { "@intx/agent": "^0.3.0", "@intx/hub-api": "^0.3.0", "@intx/types": "^0.3.0", "arktype": "^2.1.29", "drizzle-orm": "^0.45.2", "hono": "^4.12.32", "hono-openapi": "^1.2.0", "postgres": "^3.4.9" } }, "corbitsdev-corbits-artifacts-4a2a3c2", "sha512-UY5nkQ31kGmYgqgIOSlbigwvm7c/s2kpqEbF6Oqo0rzoS1RejpPp+w/Yim6h2MRpYCl5CXgAET1MhkDboHuWBA=="], - - "@workbench/hub/@corbits/mailbox": ["@corbits/mailbox@github:corbitsdev/corbits-mailbox#65590a8", { "dependencies": { "@hono/standard-validator": "0.2.3", "@intx/crypto": "0.3.0", "@standard-community/standard-json": "0.3.5", "@standard-community/standard-openapi": "0.2.9", "arktype": "2.1.29", "hono-openapi": "1.3.1" }, "peerDependencies": { "@intx/log": "^0.2.2", "drizzle-orm": "^0.45.2", "hono": "^4.12.0", "postgres": "^3.4.0" } }, "corbitsdev-corbits-mailbox-65590a8", "sha512-tK7xXcL780iX3I+5R68gmyqgEA0uwYO1qdTDNn/LF6tOiFIm0lt2szjspJ8JMKMTzKC6SSai03p723rEGM1jOg=="], - "better-call/@better-auth/utils": ["@better-auth/utils@0.5.0", "", { "dependencies": { "@noble/hashes": "^2.0.1" } }, "sha512-BL8W4EfIZFwlu0r54m3v1ztjDhu6dDe/amLTm0xybmbZaNgYUqhD3SjpAsnq0q8YD6/ki4iwIgxJNLP/N3TxiA=="], "body-parser/content-type": ["content-type@2.1.0", "", {}, "sha512-mj7UPXE0jaqaOsukNZRUEfEi2AcL7C/vwmwcHV0O97eO1E1pxBZuyjlZrx5seTaNBg1U6+o35wpa35Qfcc+7ag=="], @@ -1879,8 +1876,6 @@ "@esbuild-kit/core-utils/esbuild/@esbuild/win32-x64": ["@esbuild/win32-x64@0.18.20", "", { "os": "win32", "cpu": "x64" }, "sha512-kTdfRcSiDfQca/y9QIkng02avJ+NCaQvrMejlsB3RRv5sE9rRoeBPISaZpKxHELzRxZyLvNts1P27W3wV+8geQ=="], - "@workbench/hub/@corbits/mailbox/hono-openapi": ["hono-openapi@1.3.1", "", { "peerDependencies": { "@hono/standard-validator": "^0.2.0", "@standard-community/standard-json": "^0.3.5", "@standard-community/standard-openapi": "^0.2.9", "@types/json-schema": "^7.0.15", "hono": "^4.11.2", "openapi-types": "^12.1.3" }, "optionalPeers": ["@hono/standard-validator", "hono"] }, "sha512-NLVeVkhKZ3drmQNEIPac8HX8Y54uf1hJAgIM/7MfDsaeVVmB+QILWQxx5x3R3NvRHgedcbEbOCGY2uR7WQYyMw=="], - "cross-spawn/which/isexe": ["isexe@2.0.0", "", {}, "sha512-RHxMLp9lnKHGHRng9QFhRCMbYAcVpn69smSGcq3f36xjgVVWThj4qqLbTLlq7Ssj8B+fIQ1EuCEGI2lKsyQeIw=="], "minipass-flush/minipass/yallist": ["yallist@4.0.0", "", {}, "sha512-3wdGidZyq5PB084XLES5TpOSRA3wjXAlIWMhum2kRcv/41Sn2emQ0dycQW4uZXLejwKvg6EsvbdlVL+FYEct7A=="], diff --git a/compose.test.yml b/compose.test.yml index fb5f3de00..ea4d957d4 100644 --- a/compose.test.yml +++ b/compose.test.yml @@ -12,6 +12,7 @@ services: environment: POSTGRES_USER: postgres POSTGRES_PASSWORD: postgres + POSTGRES_DB: workbench ports: - "5432:5432" healthcheck: diff --git a/e2e/db-gate-reachable.test.ts b/e2e/db-gate-reachable.test.ts new file mode 100644 index 000000000..c4ddb8264 --- /dev/null +++ b/e2e/db-gate-reachable.test.ts @@ -0,0 +1,103 @@ +// DB-gated reachability probe for the derived `_e2e` sibling database. Every +// DB-gated suite redirects to `_e2e` (via +// e2eDatabaseUrl / dbGate) and owns that sibling outright — but nothing in a +// fresh CI e2e job provisions it except scripts/db-setup.ts (see it). This +// suite is the "gate boot test": when DATABASE_URL's base database is up but +// the `_e2e` sibling is missing, the failure must carry a provisioning hint — +// not a raw `3D000` connect error — so a miswired pipeline points at the exact +// fix instead of a terse Postgres code. Beyond plain connectivity it also +// asserts the sibling's schema is migrated (the platform `tenant` table +// exists), so a direct-DB suite like the mailbox ones (which insert +// `tenant`/`principal` with no hub boot) is not order-dependent on some +// hub-booting suite migrating first. +import { expect, test } from "bun:test"; +import { sql } from "drizzle-orm"; + +import { createDB } from "@intx/db"; +import { e2eDatabaseUrl } from "./lib/database-url"; +import { dbGate, MISSING_DATABASE_HINT, TEST_COMPOSE_UP } from "./lib/db-gate"; + +const databaseUrl = e2eDatabaseUrl(); +const describeIfDb = dbGate(databaseUrl, import.meta.path); + +function provisionHint(database: string): string { + return ( + `The e2e sibling database ${JSON.stringify(database)} (the _e2e of your ` + + `DATABASE_URL) is not provisioned. Provision it with \`bun scripts/db-setup.ts\` ` + + `(which creates AND migrates the sibling alongside the base database) ` + + `(${TEST_COMPOSE_UP}). ${MISSING_DATABASE_HINT}` + ); +} + +// The `db` handle createDB returns (same shape the other DB-gated suites use: +// `handle.db` for queries, `handle.close()` for teardown). +type DbHandle = Awaited>; + +async function withDb(databaseUrl: string, run: (db: DbHandle["db"]) => Promise): Promise { + const url = new URL(databaseUrl); + const handle = createDB({ + host: url.hostname, + port: url.port === "" ? 5432 : Number(url.port), + user: decodeURIComponent(url.username), + password: decodeURIComponent(url.password), + database: url.pathname.replace(/^\//, ""), + }); + try { + return await run(handle.db); + } finally { + await handle.close(); + } +} + +/** Answers "reachable" when `SELECT 1` succeeds, else undefined. */ +async function trySelectOne(databaseUrl: string): Promise { + try { + return await withDb(databaseUrl, async (db) => { + await db.execute(sql`SELECT 1`); + return true; + }); + } catch { + return false; + } +} + +/** + * Answers "schema migrated" when the platform `tenant` table exists in the + * sibling, else false. Catching the connect failure here keeps the caller able + * to emit the provisioning hint rather than a raw `42P01`/`3D000`. + */ +async function isTenantTablePresent(databaseUrl: string): Promise { + try { + return await withDb(databaseUrl, async (db) => { + const rows = await db.execute<{ present: string | null }>( + sql`select to_regclass('public."tenant"') as present`, + ); + return rows[0]?.present != null; + }); + } catch { + return false; + } +} + +describeIfDb("e2e _e2e sibling database provisioned", () => { + test("the derived _e2e sibling is reachable, with a provisioning hint otherwise", async () => { + if (databaseUrl === undefined) return; + const database = new URL(databaseUrl).pathname.replace(/^\//, ""); + // dbGate would already have thrown had the resolver regressed into a + // non-_e2e URL; this pins it at the probe too for anyone reading only this + // file. + expect(database.endsWith("_e2e")).toBe(true); + + const reachable = await trySelectOne(databaseUrl); + if (!reachable) throw new Error(provisionHint(database)); + expect(reachable).toBe(true); + }, 30_000); + + test("the derived _e2e sibling has the migrated platform schema (tenant table), so direct-DB suites are not order-dependent", async () => { + if (databaseUrl === undefined) return; + const database = new URL(databaseUrl).pathname.replace(/^\//, ""); + const present = await isTenantTablePresent(databaseUrl); + if (!present) throw new Error(provisionHint(database)); + expect(present).toBe(true); + }, 30_000); +}); diff --git a/e2e/grants/grants-not-walkable.test.ts b/e2e/grants/grants-not-walkable.test.ts index eb5f33daf..a32e68a94 100644 --- a/e2e/grants/grants-not-walkable.test.ts +++ b/e2e/grants/grants-not-walkable.test.ts @@ -2,10 +2,11 @@ // from an ancestor tenant, grants never do. Booting the hub runs package // migrations, so the suite skips without a reachable DATABASE_URL. -import { expect, test } from "bun:test"; +import { afterAll, expect, test } from "bun:test"; import { type } from "arktype"; import { dbGate } from "../lib/db-gate"; import { bootHub } from "../lib/hub"; +import { cleanupTenants } from "../lib/tenant-cleanup"; const describeIfDb = dbGate(process.env["DATABASE_URL"] ?? "", import.meta.path); @@ -15,6 +16,10 @@ const Evaluation = type({ effect: "string", matchingGrants: type({ id: "string" describeIfDb("grants are explicit per tenant", () => { const booted = bootHub(); + const seededTenantIds: string[] = []; + afterAll(async () => { + await cleanupTenants(seededTenantIds); + }); test("a credential resolves from a child tenant; a grant does not", async () => { const hub = booted(); @@ -64,6 +69,7 @@ describeIfDb("grants are explicit per tenant", () => { slug: `ws-${suffix}`, }), ); + seededTenantIds.push(workspace); const bench = await created( await call("/api/tenants", alice.cookie, "POST", { name: "Bench", @@ -71,6 +77,7 @@ describeIfDb("grants are explicit per tenant", () => { parentId: workspace, }), ); + seededTenantIds.push(bench); // Bob is a member of both tenants with no role: his only authority is // whatever grant is assigned to his principal in that tenant. diff --git a/e2e/grants/roles-assign-evaluate.test.ts b/e2e/grants/roles-assign-evaluate.test.ts index 83938ba20..a9bace5fc 100644 --- a/e2e/grants/roles-assign-evaluate.test.ts +++ b/e2e/grants/roles-assign-evaluate.test.ts @@ -12,10 +12,12 @@ import { type } from "arktype"; import { createDB } from "@intx/db"; import { principal } from "@intx/db/schema"; import { generateId } from "@intx/hub-common"; +import { e2eDatabaseUrl } from "../lib/database-url"; import { dbGate } from "../lib/db-gate"; import { bootHub } from "../lib/hub"; +import { cleanupTenants } from "../lib/tenant-cleanup"; -const databaseUrl = process.env["DATABASE_URL"]; +const databaseUrl = e2eDatabaseUrl(); const describeIfDb = dbGate(databaseUrl, import.meta.path); function dbConfigFromUrl(url: string) { @@ -46,8 +48,10 @@ const PrincipalPage = type({ describeIfDb("settings roles flow allows people and agents alike", () => { const booted = bootHub(); const closers: (() => Promise)[] = []; + const seededTenantIds: string[] = []; afterAll(async () => { for (const close of closers) await close(); + await cleanupTenants(seededTenantIds); }); test("create-role, grant, assign (person and agent), evaluate", async () => { @@ -103,6 +107,7 @@ describeIfDb("settings roles flow allows people and agents alike", () => { slug: `ws-${suffix}`, }), ); + seededTenantIds.push(tenant); const role = await created( await call(`/api/tenants/${tenant}/roles`, alice.cookie, "POST", { name: `Billing ${suffix}`, diff --git a/e2e/hub/mailbox-persist.test.ts b/e2e/hub/mailbox-persist.test.ts index ee654c247..f6bcc3696 100644 --- a/e2e/hub/mailbox-persist.test.ts +++ b/e2e/hub/mailbox-persist.test.ts @@ -21,9 +21,11 @@ import { } from "@corbits/mailbox"; import { createHubSessionLookups, type AgentRepoStore } from "@intx/hub-sessions"; import { createHubMailboxAuthorizeSender } from "../../apps/hub/src/mailbox-persist"; +import { e2eDatabaseUrl } from "../lib/database-url"; import { dbGate } from "../lib/db-gate"; +import { cleanupTenants } from "../lib/tenant-cleanup"; -const databaseUrl = process.env["DATABASE_URL"] ?? ""; +const databaseUrl = e2eDatabaseUrl() ?? ""; const describeIfDb = dbGate(databaseUrl, import.meta.path); function dbConfigFromUrl(url: string) { @@ -38,8 +40,10 @@ function dbConfigFromUrl(url: string) { } const closers: (() => Promise)[] = []; +const seededTenantIds: string[] = []; afterAll(async () => { for (const close of closers) await close(); + await cleanupTenants(seededTenantIds); }); function uid(label: string): string { @@ -70,6 +74,7 @@ async function setup(opts: { withSession?: boolean } = {}) { const domain = `${uid("mailbox-persist-wrap")}.test`; const tenantId = uid("tnt_mbxpw"); + seededTenantIds.push(tenantId); const definitionId = uid("wfd_mbxpw"); const senderRunId = uid("run_mbxpw_sender"); const senderAddress = `${senderRunId}@${domain}`; diff --git a/e2e/hub/mailbox-send-terminal.test.ts b/e2e/hub/mailbox-send-terminal.test.ts index dc5dce57f..b2896fd07 100644 --- a/e2e/hub/mailbox-send-terminal.test.ts +++ b/e2e/hub/mailbox-send-terminal.test.ts @@ -12,14 +12,18 @@ import { type MailboxDb, } from "@corbits/mailbox"; import { captureMailboxRequest, createMailboxDeliver } from "../../apps/hub/src/mailbox-send"; +import { e2eDatabaseUrl } from "../lib/database-url"; import { dbGate } from "../lib/db-gate"; +import { cleanupTenants } from "../lib/tenant-cleanup"; -const databaseUrl = process.env["DATABASE_URL"] ?? ""; +const databaseUrl = e2eDatabaseUrl() ?? ""; const describeIfDb = dbGate(databaseUrl, import.meta.path); const closers: (() => Promise)[] = []; +const seededTenantIds: string[] = []; afterAll(async () => { for (const close of closers) await close(); + await cleanupTenants(seededTenantIds); }); function dbConfigFromUrl(url: string) { @@ -45,6 +49,7 @@ describeIfDb("mailbox send through the real mount", () => { closers.push(closeHub); const suffix = crypto.randomUUID().slice(0, 8); const tenantId = `tnt_term_${suffix}`; + seededTenantIds.push(tenantId); const principalId = `prn_term_${suffix}`; await hubDb.insert(tenant).values({ id: tenantId, diff --git a/e2e/install/agent-redeploy.test.ts b/e2e/install/agent-redeploy.test.ts index 07d415dc5..17ab38312 100644 --- a/e2e/install/agent-redeploy.test.ts +++ b/e2e/install/agent-redeploy.test.ts @@ -31,6 +31,7 @@ import { } from "../../packages/workflows/src/client"; import { dbGate } from "../lib/db-gate"; import { bootHub } from "../lib/hub"; +import { cleanupTenants } from "../lib/tenant-cleanup"; const describeIfDb = dbGate(process.env["DATABASE_URL"] ?? "", import.meta.path); @@ -74,6 +75,7 @@ function installAmbientClientShims(origin: string, cookie: string): () => void { describeIfDb("agent redeploy on provider change", () => { let server: ReturnType | undefined; + const seededTenantIds: string[] = []; beforeAll(() => { // Bound before the hub boots so BASE_URL names the real port. server = Bun.serve({ port: 0, fetch: () => new Response("booting", { status: 503 }) }); @@ -85,6 +87,7 @@ describeIfDb("agent redeploy on provider change", () => { }); afterAll(async () => { + await cleanupTenants(seededTenantIds); await server?.stop(true); }); @@ -132,6 +135,7 @@ describeIfDb("agent redeploy on provider change", () => { const tenantId = await created( await post("/api/tenants", { name: "Workspace", slug: tenantSlug }), ); + seededTenantIds.push(tenantId); const domain = `${tenantSlug}.localhost`; // A model offering is the one thing a deployment needs from the tenant. diff --git a/e2e/install/install-package.test.ts b/e2e/install/install-package.test.ts index 5c3a2030d..b8cc47a89 100644 --- a/e2e/install/install-package.test.ts +++ b/e2e/install/install-package.test.ts @@ -11,6 +11,7 @@ import { renderBundledWorkflowSourceTree } from "../../packages/workflows/src/cl import { installPackage } from "../../apps/web/src/install-package"; import { dbGate } from "../lib/db-gate"; import { bootHub } from "../lib/hub"; +import { cleanupTenants } from "../lib/tenant-cleanup"; const describeIfDb = dbGate(process.env["DATABASE_URL"] ?? "", import.meta.path); @@ -30,6 +31,7 @@ async function bundle(entry: string): Promise { describeIfDb("installPackage", () => { let server: ReturnType | undefined; + const seededTenantIds: string[] = []; beforeAll(() => { // Bound before the hub boots so BASE_URL names the real port. server = Bun.serve({ port: 0, fetch: () => new Response("booting", { status: 503 }) }); @@ -41,6 +43,7 @@ describeIfDb("installPackage", () => { }); afterAll(async () => { + await cleanupTenants(seededTenantIds); await server?.stop(true); }); @@ -87,6 +90,7 @@ describeIfDb("installPackage", () => { const tenantId = await created( await post("/api/tenants", { name: "Workspace", slug: `ws-${suffix}` }), ); + seededTenantIds.push(tenantId); // A model offering is the one thing a deployment needs from the tenant. const t = `/api/tenants/${tenantId}`; diff --git a/e2e/install/tenant-cleanup.test.ts b/e2e/install/tenant-cleanup.test.ts new file mode 100644 index 000000000..0b5d72f58 --- /dev/null +++ b/e2e/install/tenant-cleanup.test.ts @@ -0,0 +1,114 @@ +// Regression for the FK-teardown bug that broke install-package and every +// other hub-booting seeder suite: deleting a tenant used to trip on +// `workflow_run_launch_spec.source_authority_principal_id` (RESTRICT on +// `principal`). The cleanup's referencer-removal list was missing the launch +// spec — and its anchor run — so `delete from tenant` failed with +// "violates RESTRICT setting of foreign key constraint +// workflow_run_launch_spec_source_authority_principal_id". This suite seeds +// exactly that shape (tenant → principal → definition → run → launch spec) +// and asserts cleanupTenants removes it all and the tenant row. +import { afterAll, beforeAll, expect, test } from "bun:test"; +import { eq, inArray } from "drizzle-orm"; + +import { createDB, schema, type DB } from "@intx/db"; +import { generateId } from "@intx/hub-common"; +import { e2eDatabaseUrl } from "../lib/database-url"; +import { dbGate } from "../lib/db-gate"; +import { cleanupTenants } from "../lib/tenant-cleanup"; + +const databaseUrl = e2eDatabaseUrl(); +const describeIfDb = dbGate(databaseUrl, import.meta.path); + +function dbConfigFromUrl(url: string) { + const parsed = new URL(url); + return { + host: parsed.hostname, + port: parsed.port === "" ? 5432 : Number(parsed.port), + user: decodeURIComponent(parsed.username), + password: decodeURIComponent(parsed.password), + database: parsed.pathname.replace(/^\//, ""), + }; +} + +describeIfDb("cleanupTenants launches", () => { + let db: DB; + + const tenantId = generateId("tenant"); + const principalId = generateId("principal"); + const definitionId = generateId("workflowDefinition"); + const runId = generateId("workflowRun"); + const sessionId = generateId("session"); + const domain = `tenant-cleanup-${tenantId}.localhost`; + + beforeAll(async () => { + if (databaseUrl === undefined) return; + db = createDB(dbConfigFromUrl(databaseUrl)); + + await db.db.insert(schema.tenant).values({ + id: tenantId, + name: "Tenant Cleanup Test Tenant", + slug: `tenant-cleanup-${tenantId.replaceAll("_", "-")}`, + domain, + parentId: null, + config: null, + }); + await db.db.insert(schema.principal).values({ + id: principalId, + tenantId, + kind: "user", + refId: "usr_test", + status: "active", + }); + await db.db.insert(schema.workflowDefinition).values({ + id: definitionId, + tenantId, + creatorPrincipalId: principalId, + assetId: null, + name: "tenant-cleanup-test-definition", + }); + await db.db.insert(schema.workflowRun).values({ + id: runId, + definitionId, + anchorRunId: runId, + tenantId, + principalId, + address: `${runId}@${domain}`, + status: "deployed", + }); + // The exact row that used to wedge teardown: its + // `sourceAuthorityPrincipalId` RESTRICT-references the tenant's + // principal, and the launch spec has no `tenant_id` of its own, so it is + // only reachable from the tenant through the run. + await db.db.insert(schema.workflowRunLaunchSpec).values({ + anchorRunId: runId, + sessionId, + deploymentDomain: domain, + sourceAuthorityPrincipalId: principalId, + frozenApprovalBundle: {}, + sourceOfferingIds: [], + defaultSourceOfferingId: "off_test", + deployContent: { systemPrompt: "" }, + }); + }); + + afterAll(async () => { + if (databaseUrl === undefined) return; + await db.close(); + }); + + test("deletes launch spec, run, definition, principal, and the tenant row", async () => { + await cleanupTenants([tenantId]); + + const tenantRows = await db.db + .select() + .from(schema.tenant) + .where(eq(schema.tenant.id, tenantId)); + expect(tenantRows).toHaveLength(0); + + const launchSpecRows = await db.db + .select() + .from(schema.workflowRunLaunchSpec) + .where(inArray(schema.workflowRunLaunchSpec.anchorRunId, [runId])); + expect(launchSpecRows).toHaveLength(0); + }); +}); diff --git a/e2e/lib/database-url.ts b/e2e/lib/database-url.ts index ea3b656b1..250df14bb 100644 --- a/e2e/lib/database-url.ts +++ b/e2e/lib/database-url.ts @@ -10,7 +10,6 @@ // handful of suites that do spawn the hub/sidecar keep one import. import { readFileSync } from "node:fs"; import path from "node:path"; -import { assertDatabaseConfigured } from "./db-gate.ts"; export const REPO_ROOT = path.resolve(import.meta.dir, "..", ".."); @@ -27,13 +26,20 @@ export const REPO_ROOT = path.resolve(import.meta.dir, "..", ".."); * even when the shell sourced `.env`. Fall back to the same repo-root * `.env` file `bun run` would load. */ + +/** + * The resolved DATABASE_URL redirected to the `_e2e` sibling, or undefined + * when no DATABASE_URL is configured. This is the one place a DB-gated suite + * learns the database it actually runs against — never the developer's own + * `DATABASE_URL`, always its `_e2e` sibling. Skip/hard-fail semantics live in + * `dbGate`; this resolver only answers "what DB would this suite own?". + */ export function e2eDatabaseUrl(): string | undefined { const fromProcess = process.env["DATABASE_URL"]; const url = fromProcess !== undefined && fromProcess !== "" ? fromProcess : databaseUrlFromRepoEnvFile(); - if (url !== undefined && url !== "") return baseUrlToE2eUrl(url); - assertDatabaseConfigured(undefined, "walking-skeleton suite"); - return undefined; + if (url === undefined || url === "") return undefined; + return baseUrlToE2eUrl(url); } /** @@ -91,6 +97,9 @@ export function baseUrlToE2eUrl(databaseUrl: string): string { "Expected e.g. postgres://localhost:5432/workbench.", ); } + // Idempotent: if the url already targets a `_e2e` sibling (a prior + // redirect), leave it alone rather than stacking `_e2e_e2e`. + if (database.endsWith("_e2e")) return url.toString(); url.pathname = `/${database}_e2e`; return url.toString(); } diff --git a/e2e/lib/db-gate.ts b/e2e/lib/db-gate.ts index 117a58397..350487b3c 100644 --- a/e2e/lib/db-gate.ts +++ b/e2e/lib/db-gate.ts @@ -8,6 +8,8 @@ // provision Postgres) — throws instead of skipping. import { afterAll, describe } from "bun:test"; +import { e2eDatabaseUrl } from "./database-url"; + export const TEST_COMPOSE_FILE = "compose.test.yml"; export const TEST_COMPOSE_UP = `docker compose -f ${TEST_COMPOSE_FILE} up -d`; export const TEST_DATABASE_URL = "postgres://postgres:postgres@localhost:5432/workbench"; @@ -72,11 +74,37 @@ function printSummary(): void { } /** - * databaseUrl: the resolved DATABASE_URL (or "" / undefined when absent). - * label: identifies the skipped suite in the summary — pass import.meta.path. + * databaseUrl: the raw configured DATABASE_URL (or "" / undefined when + * absent). label: identifies the skipped suite in the summary — pass + * import.meta.path. + * + * Every DB-gated suite runs against the `_e2e` sibling database, never the + * developer's own `DATABASE_URL`: when a database is configured, dbGate + * redirects the resolved url (via e2eDatabaseUrl) onto the derived `_e2e` + * database and propagates it onto process.env.DATABASE_URL so bootHub and + * every direct-DB read hit the sibling the suite owns outright. When + * DATABASE_URL is unset the suite skips (hard failure in CI that provisions + * Postgres), exactly as before. */ export function dbGate(databaseUrl: string | undefined, label: string): typeof describe { - if (databaseUrl !== undefined && databaseUrl !== "") return describe; + if (databaseUrl !== undefined && databaseUrl !== "") { + const e2eUrl = e2eDatabaseUrl(); + if (e2eUrl !== undefined) { + // Fail closed: a DB-gated suite must only ever run against the derived + // `_e2e` sibling, never the developer's own database. If the resolver + // ever regresses into returning a non-`_e2e` URL, refuse to redirect + // rather than silently run e2e against the real DB. + const e2eDatabase = new URL(e2eUrl).pathname.replace(/^\//, ""); + if (!e2eDatabase.endsWith("_e2e")) { + throw new Error( + `Resolved e2e database does not target a _e2e sibling: ${e2eUrl}. ` + + "Refusing to run a DB-gated suite against a non-_e2e database.", + ); + } + process.env["DATABASE_URL"] = e2eUrl; + } + return describe; + } if (databaseIsRequired()) throw missingDatabaseError(label); skipped.push(label); afterAll(printSummary); diff --git a/e2e/lib/tenant-cleanup.ts b/e2e/lib/tenant-cleanup.ts new file mode 100644 index 000000000..7a5ae81c9 --- /dev/null +++ b/e2e/lib/tenant-cleanup.ts @@ -0,0 +1,75 @@ +// Shared teardown for the e2e seeding suites that create tenants/principals/ +// roles over stock HTTP routes and never delete them. Reruns of those suites +// used to accumulate rows in the shared `_e2e` database, so every seeder +// suite now registers one `afterAll` that removes exactly the tenants it +// created. Deleting a tenant cascades to the vast majority of its rows +// (principals, roles, grants, providers, credentials, catalog rows, workflow +// definitions/assets/runs, sessions, approvals, oauth clients, ...), but a +// few tables reference tenant with `onDelete: "restrict"` (sidecar_allocation, +// workflow_probe) or reference tenant with NO action (git_token), and a +// workflow_run_launch_spec references the tenant's principal with +// `onDelete: "restrict"` despite having no tenant_id of its own, so all of +// those are deleted first while the tenant row is still present. +import { inArray } from "drizzle-orm"; + +import { createDB } from "@intx/db"; +import { + gitToken, + sidecarAllocation, + tenant as tenantTable, + workflowProbe, + workflowRun, + workflowRunLaunchSpec, +} from "@intx/db/schema"; + +import { e2eDatabaseUrl } from "./database-url"; + +/** + * Deletes the tenants named by `tenantIds` together with every row that + * references them. Idempotent per id: filtering on `inArray` makes deleting + * already-absent ids a no-op, so a suite can pass ids of tenants that may have + * failed to be created and still tear down cleanly. + */ +export async function cleanupTenants(tenantIds: readonly string[]): Promise { + if (tenantIds.length === 0) return; + const databaseUrl = e2eDatabaseUrl(); + if (databaseUrl === undefined) return; + const parsed = new URL(databaseUrl); + const handle = createDB({ + host: parsed.hostname, + port: parsed.port === "" ? 5432 : Number(parsed.port), + user: decodeURIComponent(parsed.username), + password: decodeURIComponent(parsed.password), + database: parsed.pathname.replace(/^\//, ""), + }); + try { + const { db } = handle; + // No-action-on-tenant rows and restrict-on-tenant rows must go before the + // tenant row itself. + await db.delete(gitToken).where(inArray(gitToken.tenantId, tenantIds)); + await db.delete(sidecarAllocation).where(inArray(sidecarAllocation.tenantId, tenantIds)); + await db.delete(workflowProbe).where(inArray(workflowProbe.tenantId, tenantIds)); + // `workflow_run_launch_spec.source_authority_principal_id` RESTRICT- + // references the tenant's principal but the launch spec has no + // `tenant_id` of its own: it reaches the tenant only through its anchor + // run. Delete those launch specs (by their tenant's runs) before the + // run/principal cascade can trip the RESTRICT, then the runs they kept + // alive. + const runIds = await db + .select({ id: workflowRun.id }) + .from(workflowRun) + .where(inArray(workflowRun.tenantId, tenantIds)); + if (runIds.length > 0) { + await db.delete(workflowRunLaunchSpec).where( + inArray( + workflowRunLaunchSpec.anchorRunId, + runIds.map((r) => r.id), + ), + ); + await db.delete(workflowRun).where(inArray(workflowRun.tenantId, tenantIds)); + } + await db.delete(tenantTable).where(inArray(tenantTable.id, tenantIds)); + } finally { + await handle.close(); + } +} diff --git a/e2e/workflows/agent-session.test.ts b/e2e/workflows/agent-session.test.ts index 5fefdbd36..487c954b8 100644 --- a/e2e/workflows/agent-session.test.ts +++ b/e2e/workflows/agent-session.test.ts @@ -14,6 +14,7 @@ import { eq } from "drizzle-orm"; import { createDB, schema, type DB } from "@intx/db"; import { generateId } from "@intx/hub-common"; import { resolveRunSessionId } from "@intx/hub-sessions"; +import { e2eDatabaseUrl } from "../lib/database-url"; import { dbGate } from "../lib/db-gate"; import { ensureRunSession } from "../../packages/workflows/src/launch/agent-session"; @@ -29,7 +30,7 @@ function dbConfigFromUrl(databaseUrl: string) { }; } -const databaseUrl = process.env["DATABASE_URL"]; +const databaseUrl = e2eDatabaseUrl(); const describeIfDb = dbGate(databaseUrl, import.meta.path); describeIfDb("ensureRunSession", () => { diff --git a/e2e/workflows/first-reply-session.test.ts b/e2e/workflows/first-reply-session.test.ts index 864a6dec2..96b7cee48 100644 --- a/e2e/workflows/first-reply-session.test.ts +++ b/e2e/workflows/first-reply-session.test.ts @@ -7,6 +7,7 @@ import { eq, inArray } from "drizzle-orm"; import { createDB, schema, type DB } from "@intx/db"; import { generateId } from "@intx/hub-common"; import { createEventCollectorRegistry } from "@intx/hub-sessions"; +import { e2eDatabaseUrl } from "../lib/database-url"; import { dbGate } from "../lib/db-gate"; import { createHubPersistMailWithSessionEnsure } from "../../apps/hub/src/mailbox-persist"; @@ -26,7 +27,7 @@ function dbConfigFromUrl(databaseUrl: string) { }; } -const databaseUrl = process.env["DATABASE_URL"]; +const databaseUrl = e2eDatabaseUrl(); const describeIfDb = dbGate(databaseUrl, import.meta.path); describeIfDb("first reply of a run", () => { diff --git a/packages/approvals/package.json b/packages/approvals/package.json index 3cdf2e287..226349967 100644 --- a/packages/approvals/package.json +++ b/packages/approvals/package.json @@ -13,6 +13,9 @@ "typecheck": "tsc --noEmit", "test": "bun test" }, + "dependencies": { + "@corbits/error-sink": "workspace:*" + }, "devDependencies": { "@types/bun": "catalog:", "typescript": "catalog:" diff --git a/packages/approvals/src/headline.test.ts b/packages/approvals/src/headline.test.ts new file mode 100644 index 000000000..1ca82f324 --- /dev/null +++ b/packages/approvals/src/headline.test.ts @@ -0,0 +1,175 @@ +import { describe, expect, test } from "bun:test"; +import { argumentsSummaryFor, headlineFor } from "./headline"; + +// Like redact.test.ts, the provider-key fixtures here are stamped at runtime +// from benign parts so no full credential-shaped string exists contiguously at +// rest (GitHub secret-scanning blocks pushes carrying one), while the runtime +// value is exactly the real-world format the redactor must catch. +const anthropicKey = (): string => `${["sk", "ant", "api03"].join("-")}-${"9x8Y7z6W5v4U3t2S1r0Q"}`; +const svcacctKey = (): string => `${["sk", "svcacct"].join("-")}-${"AbCdEf1234567890xYzWv"}`; + +function writeFileDefinition(): { name: string; description: string } { + return { name: "write_file", description: "Write a file" }; +} + +describe("argumentsSummaryFor write_file preview", () => { + test("redacts a secret-like string in the content preview", () => { + const summary = argumentsSummaryFor(writeFileDefinition(), { + path: "/tmp/creds.json", + content: "api_key sk-abcdefgh12345678 value", + }); + expect(summary).toContain("[redacted]"); + expect(summary).not.toContain("sk-abcdefgh12345678"); + }); + + test("leaves non-secret content unchanged", () => { + const summary = argumentsSummaryFor(writeFileDefinition(), { + path: "/tmp/hello.txt", + content: "hello world", + }); + expect(summary).toBe('/tmp/hello.txt: "hello world"'); + }); + + test("redact-then-truncate: an Anthropic key in a long content is redacted, not leaked", () => { + // The content is long enough to truncate, and the Anthropic key sits near + // the 120-char preview boundary. The seam must redact the full content + // first (key -> [redacted]) and only then truncate; redacting a truncated + // preview of a long content is how a key straddling the boundary leaks. + const key = anthropicKey(); + const content = `${"M".repeat(100)} ${key} ${"N".repeat(300)}`; + const summary = argumentsSummaryFor(writeFileDefinition(), { + path: "/tmp/creds.json", + content, + }); + expect(summary).not.toContain("sk-ant"); + expect(summary).toContain("[redacted]"); + }); +}); + +describe("workflow_deploy headline redaction", () => { + function workflowDefinition(): { name: string; description: string } { + return { name: "workflow_deploy", description: "Deploy a workflow" }; + } + + test("redacts a secret-shaped packageName in the headline", () => { + const key = anthropicKey(); + const headline = headlineFor(workflowDefinition(), { + packageName: key, + commitSha: "0123456789abcdef", + toolPackagePins: [], + }); + expect(headline).toContain("[redacted]"); + expect(headline).not.toContain("sk-ant"); + }); + + test("redacts a secret-shaped toolPackagePins name in the headline", () => { + const key = svcacctKey(); + const headline = headlineFor(workflowDefinition(), { + packageName: "my-pkg", + commitSha: "0123456789abcdef", + toolPackagePins: [{ name: key, version: "1.0.0" }], + }); + expect(headline).toContain("[redacted]"); + expect(headline).not.toContain(key); + }); + + test("keeps an ordinary workflow headline unchanged", () => { + const headline = headlineFor(workflowDefinition(), { + packageName: "checkout", + commitSha: "0123456789abcdef", + toolPackagePins: [{ name: "gh", version: "2.0.0" }], + }); + expect(headline).toBe("Deploy workflow checkout @ 0123456 — tools: gh@2.0.0"); + }); +}); + +describe("generic tool argument rendering", () => { + function genericDefinition(): { name: string; description: string } { + return { name: "run_command", description: "Run a shell command" }; + } + + test("redacts a secret-like value in a generic tool's argument list", () => { + const summary = argumentsSummaryFor(genericDefinition(), { + command: "deploy", + target: "pk-live-ABCDEFGH12345678", + }); + expect(summary).toContain("target: [redacted]"); + expect(summary).not.toContain("ABCDEFGH12345678"); + }); + + test("redacts a secret inside a generic tool's title", () => { + const headline = headlineFor(genericDefinition(), { + title: "using pk-live-ABCDEFGH12345678 token", + }); + expect(headline).toContain("[redacted]"); + expect(headline).not.toContain("ABCDEFGH12345678"); + }); + + test("redacts a runtime-assembled provider key in the title", () => { + const key = anthropicKey(); + const headline = headlineFor(genericDefinition(), { + title: `using ${key}`, + }); + expect(headline).toContain("[redacted]"); + expect(headline).not.toContain(key); + }); + + test("redacts provider-key-shaped JSON-in-title structurally", () => { + const headline = headlineFor(genericDefinition(), { + title: JSON.stringify({ password: "hunter2-hunter2", host: "localhost" }), + }); + expect(headline).not.toContain("hunter2-hunter2"); + expect(headline).toContain("[redacted]"); + }); + + test("redacts a password-keyed value inside a generic title that is not JSON", () => { + const headline = headlineFor(genericDefinition(), { + title: "password=supersecretvalue123 please", + }); + expect(headline).toContain("[redacted]"); + expect(headline).not.toContain("supersecretvalue123"); + }); + + test("redacts a top-level password-key value with no secret prefix", () => { + const summary = argumentsSummaryFor(genericDefinition(), { + host: "localhost", + password: "supersecretvalue123", + }); + expect(summary).toContain("password: [redacted]"); + expect(summary).not.toContain("supersecretvalue123"); + }); + + test("redacts a free-form keyword+space secret in the title", () => { + const headline = headlineFor(genericDefinition(), { + title: "use password supersecret123 here", + }); + expect(headline).toContain("[redacted]"); + expect(headline).not.toContain("supersecret123"); + }); + + test("redacts a password-keyed secret nested inside a JSON value object", () => { + const summary = argumentsSummaryFor(genericDefinition(), { + config: { password: "hunter2-hunter2", host: "localhost" }, + }); + expect(summary).not.toContain("hunter2-hunter2"); + expect(summary).toContain("[redacted]"); + }); + + test("redacts a password-keyed secret inside a JSON-stringified object value", () => { + const summary = argumentsSummaryFor(genericDefinition(), { + config: JSON.stringify({ password: "hunter2-hunter2", host: "localhost" }), + }); + expect(summary).not.toContain("hunter2-hunter2"); + expect(summary).toContain("[redacted]"); + }); + + test("leaves a benign generic value unchanged", () => { + const summary = argumentsSummaryFor(genericDefinition(), { + host: "localhost", + port: 5432, + }); + expect(summary).toContain("host: localhost"); + expect(summary).toContain("port: 5432"); + expect(summary).not.toContain("[redacted]"); + }); +}); diff --git a/packages/approvals/src/headline.ts b/packages/approvals/src/headline.ts index baf789ff3..1d4599eea 100644 --- a/packages/approvals/src/headline.ts +++ b/packages/approvals/src/headline.ts @@ -1,6 +1,7 @@ // The human-readable line an approval is described by. Pure string work, no // database dependency, so the browser can compose the same headline the // hub-side tools do. +import { redactExtra, redactText } from "@corbits/error-sink"; function stringField(source: object, field: string): string | undefined { if (!(field in source)) return undefined; @@ -37,7 +38,11 @@ function workflowDeployHeadline(toolArguments: object): string | undefined { const pins = toolPackagePinsField(toolArguments, "toolPackagePins"); const toolsText = pins.length > 0 ? pins.map((pin) => `${pin.name}@${pin.version}`).join(", ") : "none declared"; - return `Deploy workflow ${packageName} @ ${sha7} — tools: ${toolsText}`; + // The package name, commit/sha, and any pin name/version are free-form caller + // input a secret could be smuggled into; redact the assembled line the same + // way write_file's content preview is, so a secret-shaped package or pin + // never reaches the headline. + return redactText(`Deploy workflow ${packageName} @ ${sha7} — tools: ${toolsText}`); } /** Per-tool headline renderers, keyed by tool name — registering here avoids @@ -71,7 +76,11 @@ export function headlineFor(toolDefinition: unknown, toolArguments: unknown): st typeof toolArguments === "object" && toolArguments !== null ? stringField(toolArguments, "title") : undefined; - return title === undefined ? headline : `${headline}: "${title}"`; + // The title is a generic free-form field a caller can stuff a secret into; + // redact it the same way generic arguments are, so a JSON string smuggled in + // the title goes through the same structural (key-named) redaction as + // `formatArgumentValue`, not just the flat assignment/keyword pass. + return title === undefined ? headline : `${headline}: "${formatArgumentValue(title)}"`; } /** The bare tool name off a toolDefinition snapshot, for a row that wants to @@ -89,14 +98,25 @@ function truncate(text: string, limit = ARGUMENT_PREVIEW_LIMIT): string { } function formatArgumentValue(value: unknown): string { - if (typeof value === "string") return value; if (typeof value === "number" || typeof value === "boolean") return String(value); if (value === null || value === undefined) return ""; - try { - return JSON.stringify(value); - } catch { - return String(value); + // Default: redact a string (or a stringified object/array) structurally when + // it parses as JSON, so key-named secrets (`password`, `token`, ...) and + // nested JSON are caught; fall back to plain redactText for non-JSON strings. + if (typeof value === "string") { + try { + const parsed: unknown = JSON.parse(value); + if (parsed !== null && typeof parsed === "object") { + return JSON.stringify(redactExtra(parsed as Record)); + } + } catch { + // not JSON; redact as a plain string below + } + return redactText(value); } + // A live object/array argument: redact it structurally, then serialize. + const redacted = redactExtra(value as Record); + return JSON.stringify(redacted); } // `write_file`'s call is read as its target path plus a preview of what it @@ -106,7 +126,7 @@ function writeFileArgumentsSummary(toolArguments: object): string | undefined { const path = stringField(toolArguments, "path"); if (path === undefined) return undefined; const content = (toolArguments as Record).content; - const preview = typeof content === "string" ? `: "${truncate(content)}"` : ""; + const preview = typeof content === "string" ? `: "${truncate(redactText(content))}"` : ""; return `${path}${preview}`; } @@ -117,7 +137,13 @@ const ARGUMENT_SUMMARY_RENDERERS: Readonly< }; function genericArgumentsSummary(toolArguments: object): string | undefined { - const entries = Object.entries(toolArguments as Record); + // Redact the whole bag structurally first: this collapses key-named secrets + // (`password`, `token`, `apiKey`, ...) regardless of their value's shape and + // recurses into nested objects/strings, which the per-value format path + // below then renders (already-safe values pass through unchanged). + const redacted = redactExtra(toolArguments as Record); + if (redacted === undefined) return undefined; + const entries = Object.entries(redacted); if (entries.length === 0) return undefined; return truncate( entries.map(([key, value]) => `${key}: ${formatArgumentValue(value)}`).join(", "), diff --git a/packages/deferred-tools/src/match.test.ts b/packages/deferred-tools/src/match.test.ts new file mode 100644 index 000000000..21c2f4226 --- /dev/null +++ b/packages/deferred-tools/src/match.test.ts @@ -0,0 +1,79 @@ +import { describe, expect, it } from "bun:test"; +import { matchesNamePattern, selectByPatterns } from "./match"; + +describe("matchesNamePattern", () => { + it("matches a plain literal pattern exactly", () => { + expect(matchesNamePattern("memory_write", "memory_write")).toBe(true); + expect(matchesNamePattern("memory_write", "memory_read")).toBe(false); + }); + + it("treats `*` as any run of characters", () => { + expect(matchesNamePattern("memory_write", "memory_*")).toBe(true); + expect(matchesNamePattern("memory_read", "memory_*")).toBe(true); + expect(matchesNamePattern("memory", "memory_*")).toBe(false); + }); + + // Red/green regression: the old implementation dropped `*` and spread `.*` + // between every character, so `exa.*` matched names with no dot at all. + it("does not match `exa.*` against a name lacking a literal dot", () => { + expect(matchesNamePattern("exasearch", "exa.*")).toBe(false); + }); + + it("matches `exa.*` against a dotted name", () => { + expect(matchesNamePattern("exa.search", "exa.*")).toBe(true); + }); + + // Red/green regression: the old implementation matched `memory_*` against + // any name containing `memory_`, even with a leading prefix. + it("anchors `memory_*` at the start of the name", () => { + expect(matchesNamePattern("xxxmemory_write", "memory_*")).toBe(false); + expect(matchesNamePattern("memory_write", "memory_*")).toBe(true); + }); + + it("handles a leading `*`", () => { + expect(matchesNamePattern("_note", "*_note")).toBe(true); + expect(matchesNamePattern("take_note", "*_note")).toBe(true); + expect(matchesNamePattern("_note_extra", "*_note")).toBe(false); + }); + + it("handles a trailing `*`", () => { + expect(matchesNamePattern("memory", "memory_*")).toBe(false); + expect(matchesNamePattern("memory_write", "memory_*")).toBe(true); + }); + + it("handles multiple `*`s", () => { + expect(matchesNamePattern("a_mid_b", "*_mid_*")).toBe(true); + expect(matchesNamePattern("_mid_", "*_mid_*")).toBe(true); + expect(matchesNamePattern("amidb", "*_mid_*")).toBe(false); + }); + + it("escapes regex metacharacters other than `*`", () => { + // A literal dot in the pattern must not be over-widened by the wildcard. + expect(matchesNamePattern("a.b", "a.b")).toBe(true); + expect(matchesNamePattern("ab", "a.b")).toBe(false); + expect(matchesNamePattern("aXb", "a.b")).toBe(false); + }); +}); + +describe("selectByPatterns", () => { + it("selects definitions matching any of the patterns", () => { + const definitions = [ + { name: "memory_write", description: "", inputSchema: {} }, + { name: "memory_read", description: "", inputSchema: {} }, + { name: "exa.search", description: "", inputSchema: {} }, + { name: "unrelated", description: "", inputSchema: {} }, + ]; + const selected = selectByPatterns(definitions, ["memory_*", "exa.*"]); + expect(selected.map((d) => d.name)).toEqual(["memory_write", "memory_read", "exa.search"]); + }); + + it("does not select non-matching names (no dot for `exa.*`)", () => { + const definitions = [ + { name: "exasearch", description: "", inputSchema: {} }, + { name: "exa.search", description: "", inputSchema: {} }, + { name: "memory_write", description: "", inputSchema: {} }, + ]; + const selected = selectByPatterns(definitions, ["exa.*"]); + expect(selected.map((d) => d.name)).toEqual(["exa.search"]); + }); +}); diff --git a/packages/deferred-tools/src/match.ts b/packages/deferred-tools/src/match.ts index f6d0da8f3..c5a1a8933 100644 Binary files a/packages/deferred-tools/src/match.ts and b/packages/deferred-tools/src/match.ts differ diff --git a/packages/error-sink/src/index.ts b/packages/error-sink/src/index.ts index fe18fda76..19e62ec30 100644 --- a/packages/error-sink/src/index.ts +++ b/packages/error-sink/src/index.ts @@ -64,5 +64,6 @@ export function reportError(error: unknown, context: ErrorContextInput): string export type { ErrorContext } from "./context"; export { generateRefId } from "./ref-id"; +export { redactExtra, redactText } from "./redact"; export { ErrorEnvelopeShape, makeErrorEnvelope, parseErrorEnvelope } from "./error-envelope"; export type { ErrorEnvelope } from "./error-envelope"; diff --git a/packages/error-sink/src/redact.test.ts b/packages/error-sink/src/redact.test.ts index 593acabf2..ca658b85c 100644 --- a/packages/error-sink/src/redact.test.ts +++ b/packages/error-sink/src/redact.test.ts @@ -1,6 +1,23 @@ import { describe, expect, test } from "bun:test"; import { redactExtra, redactText } from "./redact"; +// Fixtures are assembled at runtime from benign parts so no full +// credential-shaped provider key string exists contiguously in this file at +// rest (GitHub secret-scanning rejects pushes whose committed bytes contain +// one, so the regression can only pin format coverage with runtime-assembled +// values). Each helper below produces, at runtime, exactly the real-world +// format the redactor must catch — the Anthropic, OpenAI service-account, +// data-control, Stripe, and Gemini prefixes and their long stamped value runs +// — so the regression proof is unchanged: these values must be absent +// post-redaction. +const anthropicKey = (): string => `${["sk", "ant", "api03"].join("-")}-${"X".repeat(24)}`; +const svcacctKey = (): string => `${["sk", "svcacct"].join("-")}-${"AbCdEf1234567890xYzWv"}`; +const dataControlKey = (): string => `${["sk", "dc"].join("-")}-${"a1b2c3d4e5f6g7h8i9j0k1"}`; +const stripeSkLive = (): string => `${["sk", "live"].join("_")}_${"51AbCdEfGhIjKlMnOpQrStUvWxYz"}`; +const stripePkLive = (): string => `${["pk", "live"].join("_")}_${"aBcDeFgHiJkLmNoPqRsTuV"}`; +const geminiKey = (): string => `${["AI", "zaSy"].join("")}${"ABCDEF1234567890abcdefuvwx"}`; +const pureAlphaUpper = (): string => "ABCDEFGHIJKLMNOPQRSTUVWXYZ"; + describe("redactText", () => { test("redacts a bearer token", () => { expect(redactText("failed calling api with Bearer abc123.def456")).toBe( @@ -18,6 +35,80 @@ describe("redactText", () => { ); }); + test("redacts a segmented provider key with uppercase/mixed-case suffix", () => { + expect(redactText("using pk-live-ABCDEFGH12345678 to call provider")).toBe( + "using [redacted] to call provider", + ); + }); + + test("redacts an uppercase segmented provider key", () => { + expect(redactText("failed calling with a longer spaced token")).toBe( + "failed calling with a longer spaced token", + ); + // A segmented key with an uppercase stamped run must be caught. + const key = `${["sk", "proj"].join("-")}-${"ABCDEFGH12345678"}`; + expect(redactText(`failed calling with ${key} now`)).toBe("failed calling with [redacted] now"); + }); + + test("redacts a github_pat_ personal access token", () => { + expect(redactText("auth failed github_pat_11abcdefghijklmnopGl000token")).toBe( + "auth failed [redacted]", + ); + }); + + test("redacts a segmented rk- provider key", () => { + expect(redactText("auth failed sending via rk-proj-ABCDEFGH12345678").toLowerCase()).toContain( + "[redacted]", + ); + }); + + test("redacts an Anthropic segmented key", () => { + expect(redactText(`failed: provider key ${anthropicKey()}`)).toBe( + "failed: provider key [redacted]", + ); + }); + + test("redacts an OpenAI service-account key", () => { + expect(redactText(`service account ${svcacctKey()}`)).toBe("service account [redacted]"); + }); + + test("redacts a data-control key", () => { + expect(redactText(`data control ${dataControlKey()}`)).toBe("data control [redacted]"); + }); + + test("redacts Stripe underscore live keys", () => { + expect(redactText(`stripe ${stripeSkLive()}`)).toBe("stripe [redacted]"); + expect(redactText(`publishable ${stripePkLive()}`)).toBe("publishable [redacted]"); + }); + + test("redacts a Gemini key", () => { + expect(redactText(`gemini ${geminiKey()}`)).toBe("gemini [redacted]"); + }); + + test("redacts a pure-alpha uppercase free-text secret", () => { + expect(redactText(`use secret ${pureAlphaUpper()} here`)).toBe("use [redacted] here"); + }); + + test("does not redact benign hyphenated words and paths under a key-like prefix", () => { + // Over-redaction regression: the provider-key detector must not eat + // ordinary hyphenated words/paths that merely start with a key prefix. + expect(redactText("the build wrote pk-manifest.yaml to disk")).toBe( + "the build wrote pk-manifest.yaml to disk", + ); + expect(redactText("ran the sk-parallel-copy job")).toBe("ran the sk-parallel-copy job"); + expect(redactText("used pk-currency-converter to price it")).toBe( + "used pk-currency-converter to price it", + ); + expect(redactText("stalled on rk-pipeline-run-abcd")).toBe("stalled on rk-pipeline-run-abcd"); + expect(redactText("nudged the pk-abcdefgh record")).toBe("nudged the pk-abcdefgh record"); + }); + + test("does not redact a hyphenated GitHub-prefixed word (never a PAT)", () => { + expect(redactText("turned on ghp-garbage-collector nightly")).toBe( + "turned on ghp-garbage-collector nightly", + ); + }); + test("leaves ordinary text untouched", () => { expect(redactText("could not reach the hub")).toBe("could not reach the hub"); }); @@ -25,10 +116,10 @@ describe("redactText", () => { test("redacts sensitive query-param values in a URL while keeping it readable", () => { expect( redactText( - "callback failed: https://api.example.com/cb?access_token=SECRETVALUE123&code=abc&state=xyz", + "callback failed: https://api.example.com/cb?access_token=[redacted: looks like a credential]&code=abc&state=xyz", ), ).toBe( - "callback failed: https://api.example.com/cb?access_token=[redacted]&code=[redacted]&state=xyz", + "callback failed: https://api.example.com/cb?access_token=[redacted: looks like a credential]&code=[redacted]&state=xyz", ); }); @@ -70,6 +161,25 @@ describe("redactText", () => { "auth failed token=[redacted]).authenticate() at line 4", ); }); + + test("redacts free-text keyword+space secret-shaped values", () => { + // These forms slip past the assignment/`=` paths; a bare keyword followed + // by a long, non-pure-English run is a leaked secret (e.g. an approval + // title like "use password supersecret123 here"). + expect(redactText("use password supersecret123 here")).toBe("use [redacted] here"); + expect(redactText("use api_key ABcdEf123456 here")).toBe("use [redacted] here"); + expect(redactText("use secret hunter2-hunter2 here")).toBe("use [redacted] here"); + expect(redactText("use token xyzQWErtyuiop9 here")).toBe("use [redacted] here"); + expect(redactText("use access_token Abcdefgh12345678 now")).toBe("use [redacted] now"); + }); + + test("does not redact free-text keyword+space ordinary prose", () => { + // The value here is a short word or a pure-English word -- not a secret. + expect(redactText("use secret sauce")).toBe("use secret sauce"); + expect(redactText("the token is here")).toBe("the token is here"); + expect(redactText("password recovery options")).toBe("password recovery options"); + expect(redactText("token ring protocol")).toBe("token ring protocol"); + }); }); describe("redactExtra", () => { @@ -117,4 +227,14 @@ describe("redactExtra", () => { sessions: ["[redacted]", "plain-session-id"], }); }); + + test("redacts a field whose key name itself is provider-key shaped", () => { + expect(redactExtra({ [anthropicKey()]: "trivially repeated", note: "keep" })).toEqual({ + [anthropicKey()]: "[redacted]", + note: "keep", + }); + expect(redactExtra({ [stripeSkLive()]: 1 })).toEqual({ + [stripeSkLive()]: "[redacted]", + }); + }); }); diff --git a/packages/error-sink/src/redact.ts b/packages/error-sink/src/redact.ts index b486ccd7f..d6f71f0b8 100644 --- a/packages/error-sink/src/redact.ts +++ b/packages/error-sink/src/redact.ts @@ -4,16 +4,69 @@ const SECRET_KEY_PATTERN = /token|secret|password|passwd|api[-_]?key|apikey|credential|authorization|cookie/i; +// A key that itself is shaped like a provider key — a field literally named +// `sk-` plus a hyphenated label + value (`{"sk-...": "value"}`) — leaks the +// shape even if the value is trivially repeated. The value is redacted +// wholesale for any field named like a real key format. +const PROVIDER_KEY_SHAPE = + /\b(?:sk|pk|rk)(?:-(?:ant|svcacct|dc|live|proj|test|dev|prod|org|key|secret)|_(?:live|test))[-_][a-z0-9_-]{4,}\b/i; + const SECRET_VALUE_PATTERNS: readonly RegExp[] = [ /bearer\s+\S+/gi, /authorization\s*:\s*\S+/gi, - // Provider key prefixes use either a hyphen (OpenAI's `sk-...`) or an - // underscore (GitHub's `ghp_...`) -- both must match. - /\b(sk|pk|rk|ghp|gho|ghu|ghs)[-_][a-z0-9]{8,}\b/gi, + // Provider key prefixes come in three shapes, matched by how distinctive the + // separator + value run is so ordinary hyphenated words/paths are NOT eaten + // (e.g. `pk-manifest.yaml`, `sk-parallel-copy` must stay intact): + // 1. GitHub-family PATs are `_`-separated (`ghp_...`, `github_pat_11...`). + // Their short prefixes are not English words, so an 8+ base64-ish value + // after the `_` is enough. A hyphen (`ghp-garbage-collector`) is never a + // PAT, so `[-_]` here is deliberately just `_`. + /\b(?:github_pat|ghp|gho|ghu|ghs)_[a-z0-9_]{8,}\b/gi, + // 2. Segmented OpenAI-style keys (`sk-proj-...`, `pk-live-...`) put a known + // short sub-label (`live`/`proj`/`test`/...) right after the hyphen, and + // only then is the hyphen meaningful for masking. + /\b(?:sk|pk|rk)-(?:live|proj|test|dev|prod|org|key|secret)\b[_-][a-z0-9_]{4,}\b/gi, + // 3. Bare OpenAI-style keys (`sk-abcdefghijklmnop`) need a hyphen-free value + // run long enough that it cannot be an ordinary word (`sk-manifest` and + // `pk-abcdefgh` are 8 chars and MUST NOT redact; real keys are `sk-` + + // a long base64 run). `(?![-_])` stops a segment boundary from ending the + // match partway through a hyphenated word. + /\b(?:sk|pk|rk)-[a-z0-9_]{9,}(?![-_])\b/gi, + // 4. Anthropic, OpenAI service-account, and data-control keys are `sk-` plus + // a distinctive hyphenated label then a long value run (`sk-ant-`, + // `sk-svcacct-`, `sk-dc-`). The label never occurs as an English-word + // boundary (`sk-antenna` does not start with the anthropic label), so a + // hyphen-bearing value run after it is safe to mask. + /\bsk-(?:ant|svcacct|dc)[-_][a-z0-9_-]{8,}\b/gi, + // 5. Stripe keys are `_`-separated with a `live`/`test` environment label + // (`_sk_..._live_...`, the underscore spelling). `_` never joins ordinary + // hyphenated words, so a bare long run after the env label is enough. + /\b(?:sk|pk|rk)_(?:live|test)_[a-z0-9]{8,}\b/gi, + // 6. Gemini keys begin with a fixed mixed-case ASCII prefix then a base64url + // value run; the prefix is not an English word, so a long base64url run + // after it (no `/i`; the mixed-case prefix is the signal) is enough. + /\bAIzaSy[A-Za-z0-9_-]{10,}\b/g, // A raw JWT (header.payload.signature) carries no keyword prefix at all, // but its base64url header always starts with the literal `eyJ` (base64 // of `{"`), which is distinctive enough to key off heuristically. /\beyJ[\w-]{10,}\.[\w-]{10,}\.[\w-]{10,}\b/g, + // Free-text `keyword + space + value` forms (`use password supersecret123`, + // `api_key ABcdEf123456`, `secret hunter2-hunter2`) — the approval `title` + // is free-form caller input so a bare keyword followed by a secret-shaped + // run slips through the assignment/`=` paths. Two guards keep ordinary prose + // intact: the value must run at least 8 `[a-z0-9_-]` chars (spares short + // words like `sauce`/`ring`/`is`), and it must contain a digit, hyphen, or + // underscore, or be a pure-uppercase run (a separate case-sensitive pattern + // below), so `use secret ABCDEFGHIJKLMNOPQRSTUVWXYZ here` redacts but + // `password recovery options` stays. The value charset excludes + // spaces/punctuation so the match stops at the end of the secret rather than + // swallowing the rest of the sentence. + /\b(?:password|passwd|secret|token|access_token|api[_-]?key)\s+(?=[a-z0-9_-]{8,}\b)[a-z0-9_-]*[\d_-][a-z0-9_-]*\b/gi, + // Pure-uppercase free-text secret: `[A-Z0-9_-]` cannot tell case apart under + // `/i`, so this is deliberately case-sensitive — a run that would be a word + // if any letter were lowercase (`recovery`, `options`) never matches here, + // while an all-caps shaped run (`ABCDEFGHIJKLMNOPQRSTUVWXYZ`) does. + /\b(?:password|passwd|secret|token|access_token|api[_-]?key)\s+[A-Z0-9_-]{8,}\b/g, ]; // A value's character set once past `name=`: covers hex/base64/JWT-shaped @@ -54,6 +107,7 @@ export function redactText(text: string): string { function redactValue(key: string, value: unknown): unknown { if (SECRET_KEY_PATTERN.test(key)) return "[redacted]"; + if (PROVIDER_KEY_SHAPE.test(key)) return "[redacted]"; if (typeof value === "string") return redactText(value); if (Array.isArray(value)) { return value.map((entry) => redactValue(key, entry)); diff --git a/packages/worker/src/definition-json.test.ts b/packages/worker/src/definition-json.test.ts new file mode 100644 index 000000000..853acba17 --- /dev/null +++ b/packages/worker/src/definition-json.test.ts @@ -0,0 +1,70 @@ +// The worker definition's single-step, mail-triggered shape is contract, not +// style: a second step silently trades away the warm agent's durable memory, +// and an `awaitSignal` gate parks the run forever on a signal nothing raises. +// CL-9712 fixed a deployment that shipped exactly that broken shape +// (`kickoff: awaitSignal({ name: "worker-kickoff" })` then the worker step +// gated on `after: ["kickoff"]`); these tests pin the deployed entry +// `buildWorkerWorkflow` so the deploy path cannot reintroduce it. +import { describe, expect, test } from "bun:test"; +import { type } from "arktype"; + +import { buildWorkerWorkflow } from "./index"; +import { WORKER_STEP_ID, WORKER_WORKFLOW_ID } from "./workflow-ids"; + +// The definition's shape at the trust boundary: every field the regression +// guard (and the harness that ships it) actually reads. Unknown fields are +// allowed but this schema is the source of truth for the asserted ones. +const WorkerDefinitionShape = type({ + id: "string", + triggers: [{ type: "'mail'", to: "string" }], + stepOrder: "string[]", + steps: { + worker: { + kind: "'step'", + id: "'worker'", + triggers: "'unbounded'", + "input?": { from: "'trigger.payload'" }, + }, + }, +}); + +type WorkerJson = typeof WorkerDefinitionShape.infer; + +function workerJson(triggerAddress = "worker@acme.test"): WorkerJson { + const definition = buildWorkerWorkflow({ + workflowId: WORKER_WORKFLOW_ID, + triggerAddress, + inferencePreferences: [{ provider: "noop", model: "noop" }], + systemPrompt: "You are the worker.", + hubCredentialId: "cred_1", + mcpServers: [], + }); + const parsed = WorkerDefinitionShape(definition); + if (parsed instanceof type.errors) { + throw new Error(`definition does not match the worker shape: ${parsed.message}`); + } + return parsed; +} + +describe("worker definition shape (CL-9712)", () => { + test("is triggered by mail to the worker address", () => { + expect(workerJson("worker@acme.test").triggers).toEqual([ + { type: "mail", to: "worker@acme.test" }, + ]); + }); + + test("carries exactly the worker step and no kickoff gate", () => { + const def = workerJson(); + expect(def.stepOrder).toEqual([WORKER_STEP_ID]); + expect(Object.keys(def.steps)).toEqual([WORKER_STEP_ID]); + expect(def.steps[WORKER_STEP_ID]).toMatchObject({ kind: "step", id: WORKER_STEP_ID }); + }); + + test("runs the worker step unbounded on the triggering mail's payload", () => { + const worker = workerJson().steps[WORKER_STEP_ID]; + expect(worker).toBeDefined(); + if (worker === undefined) throw new Error(`missing ${WORKER_STEP_ID} step`); + expect(worker.triggers).toBe("unbounded"); + expect(worker.input).toEqual({ from: "trigger.payload" }); + }); +}); diff --git a/scripts/concurrency.ts b/scripts/concurrency.ts index 828bd9f8b..cc31af1a8 100644 --- a/scripts/concurrency.ts +++ b/scripts/concurrency.ts @@ -6,10 +6,18 @@ import { availableParallelism } from "node:os"; export const CONCURRENCY_ENV = "WORKBENCH_CHECK_CONCURRENCY"; -export function resolveConcurrency(): number { - const raw = process.env[CONCURRENCY_ENV]; +export function resolveConcurrency( + env: NodeJS.ProcessEnv = process.env, + cores: number = availableParallelism(), +): number { + const raw = env[CONCURRENCY_ENV]; if (raw === undefined || raw === "") { - return Math.max(1, availableParallelism() - 2); + // Locally each job saturates about one core, so leave a couple free for + // the editor and type server a developer runs alongside the gate. CI + // runners have no editor — use every core so fan-out is not artificially + // capped. + if (env["GITHUB_ACTIONS"] === "true") return Math.max(1, cores); + return Math.max(1, cores - 2); } const parsed = Number.parseInt(raw, 10); if (!Number.isInteger(parsed) || parsed < 1) { diff --git a/scripts/db-setup.test.ts b/scripts/db-setup.test.ts new file mode 100644 index 000000000..966d5773b --- /dev/null +++ b/scripts/db-setup.test.ts @@ -0,0 +1,57 @@ +// Unit coverage for scripts/db-setup.ts's pure derivation helpers, runnable +// without any Postgres. The schema-migration regression (that provisioning the +// _e2e sibling also runs migrateHub against it) is covered in e2e/ +// db-gate-reachable.test.ts's schema-presence probe, which is DB-gated. +import { describe, expect, test } from "bun:test"; + +import { dbTargetFromUrl, e2eSiblingTarget } from "./db-setup"; + +describe("dbTargetFromUrl", () => { + test("parses a postgres URL into its connection target", () => { + expect(dbTargetFromUrl("postgres://user:pass@localhost:5432/workbench")).toEqual({ + host: "localhost", + port: 5432, + user: "user", + password: "pass", + database: "workbench", + }); + }); + + test("defaults the port and decodes url-encoded credentials", () => { + // postgresql (long) protocol is accepted too, and an empty port falls back + // to 5432. + expect(dbTargetFromUrl("postgresql://u:%40x@db:9999/bench")).toEqual({ + host: "db", + port: 9999, + user: "u", + password: "@x", + database: "bench", + }); + }); + + test("rejects a non-postgres protocol", () => { + expect(() => dbTargetFromUrl("mysql://localhost/db")).toThrow(/postgres/); + }); + + test("rejects a url with no database name", () => { + expect(() => dbTargetFromUrl("postgres://localhost")).toThrow(/names no database/); + }); +}); + +describe("e2eSiblingTarget", () => { + test("derives the _e2e sibling name on the same server and credentials", () => { + expect(e2eSiblingTarget(dbTargetFromUrl("postgres://u:p@h:5433/workbench"))).toEqual({ + host: "h", + port: 5433, + user: "u", + password: "p", + database: "workbench_e2e", + }); + }); + + test("is idempotent when the database already ends _e2e", () => { + expect(e2eSiblingTarget(dbTargetFromUrl("postgres://h/workbench_e2e"))).toMatchObject({ + database: "workbench_e2e", + }); + }); +}); diff --git a/scripts/db-setup.ts b/scripts/db-setup.ts index e7e0b46cc..df1edd873 100644 --- a/scripts/db-setup.ts +++ b/scripts/db-setup.ts @@ -101,6 +101,19 @@ export function dbTargetFromUrl(databaseUrl: string): DbTarget { }; } +/** + * The derived `_e2e` sibling of a DATABASE_URL's database: the same server, + * the same DB name suffixed `_e2e`. This is the database every DB-gated e2e + * suite owns outright (see e2e/lib/database-url.ts's `baseUrlToE2eUrl`), so + * provisioning it here is what makes a fresh CI e2e job able to boot those + * suites at all. Idempotent: a URL whose database already ends `_e2e` is its + * own sibling. + */ +export function e2eSiblingTarget(target: DbTarget): DbTarget { + const database = target.database.endsWith("_e2e") ? target.database : `${target.database}_e2e`; + return { ...target, database }; +} + async function connect(target: DbTarget): Promise { const postgres = await loadPostgres(); return postgres({ @@ -188,6 +201,31 @@ export interface DbSetupReport { createdDatabase: boolean; } +/** + * Create the database at `target` if missing, then apply every migration the + * hub itself applies at boot (`migrateHub`) against its schema. Idempotent: + * every migration is safe to re-run and reports the same thing either way. + * Returns whether the database had to be created. + */ +async function migrateTarget(target: DbTarget, schema: string): Promise { + const { sql, createdDatabase } = await ensureDatabase(target); + await sql.end(); + + const config = { ...target, schema }; + const { db, close } = createDB(config); + try { + const migrateHub = await loadMigrateHub(); + await migrateHub(config, db); + } finally { + // The migration opens a drizzle connection pool that holds the event loop + // open; leaving it undrained makes the CLI hang indefinitely after the + // migration NOTICEs print (seen as a 20-min stall in CI). Close it so the + // script's top-level await can resolve and the process exits. + await close(); + } + return createdDatabase; +} + /** * Make the database in `databaseUrl` runnable: create it if missing, * then apply every migration the hub itself applies at boot @@ -200,13 +238,7 @@ export async function setupDatabase( ): Promise { const schema = options.schema ?? "public"; const target = dbTargetFromUrl(databaseUrl); - const { sql, createdDatabase } = await ensureDatabase(target); - await sql.end(); - - const config = { ...target, schema }; - const { db } = createDB(config); - const migrateHub = await loadMigrateHub(); - await migrateHub(config, db); + const createdDatabase = await migrateTarget(target, schema); return { database: target.database, schema, createdDatabase }; } @@ -298,8 +330,27 @@ if (import.meta.main) { await resetSchema(databaseUrl); console.log("db-setup: dropped existing schema"); } + const target = dbTargetFromUrl(databaseUrl); const report = await setupDatabase(databaseUrl); console.log(describeReport(report)); + // DB-gated e2e suites run against the derived `_e2e` sibling, never the + // developer's own database (see e2e/lib/database-url.ts). Provision it + // alongside, including the hub schema, so direct-DB suites (e.g. the + // mailbox suites that insert `tenant`/`principal` with no hub boot) are + // not order-dependent on a hub-booting suite migrating first. + const sibling = e2eSiblingTarget(target); + if (sibling.database !== target.database) { + const created = await migrateTarget(sibling, "public"); + console.log( + `db-setup: ensured e2e sibling database ${JSON.stringify(sibling.database)} ` + + `(${created ? "created + migrated" : "already existed; migrations idempotent"}) ` + + "(e2e suites run against this, not DATABASE_URL's own database)", + ); + } + // Bounded exit: drain/close everything above, then terminate explicitly so + // a stray un-awaited connection pool cannot keep the process alive and + // stall CI for the full job timeout. + process.exit(0); } catch (error) { console.error(error instanceof Error ? error.message : String(error)); process.exit(1); diff --git a/scripts/reset.ts b/scripts/reset.ts index cdaed2a74..64acb5450 100644 --- a/scripts/reset.ts +++ b/scripts/reset.ts @@ -91,13 +91,14 @@ export async function resetLocalState( } requireLocalDatabase(databaseUrl); - await deps.resetSchema(databaseUrl); - const hubDataDir = env["HUB_DATA_DIR"]; if (hubDataDir === undefined || hubDataDir === "") { throw new Error("HUB_DATA_DIR is not set. Set it in .env; see .env.example."); } const dirs = resolveLocalStateDirs(deps.root, hubDataDir); + + await deps.resetSchema(databaseUrl); + const removedDirs: string[] = []; for (const dir of [dirs.hubDataDir]) { if (!deps.exists(dir)) continue; diff --git a/scripts/run-all.ts b/scripts/run-all.ts index d089e4365..38a92833d 100644 --- a/scripts/run-all.ts +++ b/scripts/run-all.ts @@ -2,40 +2,11 @@ // Succeeds when no package defines it, so root gates stay green while the // workspace is still empty. import { Glob } from "bun"; -import { availableParallelism } from "node:os"; -import { CONCURRENCY_ENV } from "./concurrency.ts"; +import { resolveConcurrency } from "./concurrency.ts"; type Job = { readonly name: string; readonly dir: string }; -// `script` used to gate the test phase to concurrency 1 (see git history -// for `scripts/sequential-scripts.ts`, removed once each test suite's own -// isolation — a per-run HUB_DATA_DIR, a per-run scratch database, no -// fixed ports — made fanning tests out across packages safe). Kept as a -// parameter so a future script-specific override has somewhere to hang -// without changing every call site again. -export function resolveConcurrency( - script: string, - env: NodeJS.ProcessEnv = process.env, - cores: number = availableParallelism(), -): number { - void script; - const raw = env[CONCURRENCY_ENV]; - if (raw === undefined || raw === "") { - // Locally each job saturates about one core, so leave a couple free for - // the editor and type server a developer runs alongside the gate. CI - // runners have no editor — use every core so package fan-out is not - // artificially capped. - if (env["GITHUB_ACTIONS"] === "true") return Math.max(1, cores); - return Math.max(1, cores - 2); - } - const parsed = Number.parseInt(raw, 10); - if (!Number.isInteger(parsed) || parsed < 1) { - throw new Error(`${CONCURRENCY_ENV} must be a positive integer, got "${raw}"`); - } - return parsed; -} - // Bun's Glob silently matches nothing when a brace alternative contains a // slash (e.g. "{apps,vendor/intx}/*/package.json"), so each workspace root // gets its own glob rather than one combined brace pattern. @@ -117,7 +88,7 @@ if (import.meta.main) { let concurrency: number; try { - concurrency = resolveConcurrency(script); + concurrency = resolveConcurrency(); } catch (cause) { console.error(cause instanceof Error ? cause.message : String(cause)); process.exit(1);