Fix review findings: 204 void contract, redaction gaps, glob-anchored name matching

.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

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

apps/web/src/settings/api-request.ts

@@ -42,11 +42,45 @@ export async function apiRequest<T>(
     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) {
     throw new ErrorCtor(`Unexpected response shape while ${verb}: ${parsed.summary}`);
   }
   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<T>` caller
+ * having to accept an unsound `undefined as T`. */
+export async function requestVoid(
+  path: string,
+  verb: string,
+  ErrorCtor: ApiErrorCtor,
+  init?: RequestInit,
+): Promise<void> {
+  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,
+    );
+  }
+}

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<void> {
-  return request<void>(
+  return requestVoid(
     `/api/tenants/${tenantId}/credentials/${credentialId}`,
-    (data) => data as void,
     "revoking that credential",
+    CredentialsApiError,
     { method: "DELETE" },
   );
 }

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();
+  });
+});

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;
   }

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();
+  });
+});