fix(api): address review feedback

Dockerfile

@@ -1,4 +1,4 @@
-FROM node:24-bookworm-slim@sha256:0e0ff40c39bc087845bfb27465a0df4ea419520094bc35842ff83dd8cbe6f9b6 AS build
+FROM --platform=$BUILDPLATFORM node:24-bookworm-slim@sha256:0e0ff40c39bc087845bfb27465a0df4ea419520094bc35842ff83dd8cbe6f9b6 AS build
 WORKDIR /app
 COPY package.json package-lock.json ./
 COPY packages/shared/package.json packages/shared/package.json
@@ -10,7 +10,7 @@ RUN npm run build:shared && npm run build -w @seahaven-ap/api
 ARG GIT_SHA=unknown
 RUN GIT_SHA="$GIT_SHA" node -e "require('node:fs').writeFileSync('packages/api/dist/build-info.js', 'export const BUILD_GIT_SHA = ' + JSON.stringify(process.env.GIT_SHA || 'unknown') + ';\\n')"
 
-FROM node:24-alpine@sha256:ebfe2f90462722a7a4de65e91990e97fe0d401c70e0e762c5b53302f905ec1c1
+FROM --platform=linux/amd64 node:24-alpine@sha256:ebfe2f90462722a7a4de65e91990e97fe0d401c70e0e762c5b53302f905ec1c1
 RUN addgroup -S app && adduser -S -G app app
 WORKDIR /app
 COPY package.json package-lock.json ./

packages/api/src/app.ts

@@ -12,7 +12,7 @@ import { createUserRoutes } from "./routes/users.js";
 import { createInvoiceRoutes } from "./routes/invoices.js";
 import { createApprovalRoutes } from "./routes/approvals.js";
 import { createDocumentsStore, type DocumentsStore } from "./documents.js";
-import { errorJson } from "./http.js";
+import { ApiError, errorJson } from "./http.js";
 import { cloudFrontOriginAllowed } from "./auth/origin-verify.js";
 import { csrfAllowed, isMutating } from "./auth/oauth.js";
 import type { CognitoTokenClient } from "./auth/cognito.js";
@@ -58,6 +58,9 @@ export function createApp(env: ApiEnv, handle: Db, deps: AppDeps = {}) {
 
   app.notFound((c) => errorJson(c, 404, "NOT_FOUND", "Not found."));
   app.onError((error, c) => {
+    if (error instanceof ApiError) {
+      return errorJson(c, error.status, error.code, error.message);
+    }
     console.error(error);
     return errorJson(c, 500, "INTERNAL_ERROR", "Internal server error.");
   });

packages/api/src/approvals.ts

@@ -1,5 +1,5 @@
 import { eq } from "drizzle-orm";
-import type { Db } from "./db/client.js";
+import type { DbHandle } from "./db/client.js";
 import { activityLog, approvalPolicies, approvalSteps, invoices } from "./db/schema/index.js";
 import { moneyCents, rowsOf } from "./routes/helpers.js";
 
@@ -8,7 +8,7 @@ type PolicyRow = typeof approvalPolicies.$inferSelect;
 type StepRow = typeof approvalSteps.$inferSelect;
 
 export async function appendActivity(
-  handle: Db,
+  handle: DbHandle,
   invoiceId: string,
   actorUserId: string,
   message: string,
@@ -17,7 +17,7 @@ export async function appendActivity(
 }
 
 export async function applyMatchingPolicy(
-  handle: Db,
+  handle: DbHandle,
   invoice: InvoiceRow,
   actorUserId: string,
 ): Promise<InvoiceRow> {
@@ -68,7 +68,28 @@ export async function applyMatchingPolicy(
   return updated ?? { ...invoice, status: "approved" };
 }
 
-export async function remainingPending(handle: Db, invoiceId: string): Promise<StepRow[]> {
+export async function remainingPending(handle: DbHandle, invoiceId: string): Promise<StepRow[]> {
   const rows = await rowsOf<StepRow>(handle, approvalSteps);
   return rows.filter((row) => row.invoiceId === invoiceId && row.status === "pending");
 }
+
+export async function closePendingSteps(
+  handle: DbHandle,
+  invoiceId: string,
+  actorUserId: string | null,
+): Promise<void> {
+  const pending = await remainingPending(handle, invoiceId);
+  const actedAt = new Date();
+  for (const step of pending) {
+    await handle.db
+      .update(approvalSteps)
+      .set({
+        id: step.id,
+        status: "skipped",
+        actedByUserId: actorUserId,
+        actedAt,
+      })
+      .where(eq(approvalSteps.id, step.id))
+      .returning();
+  }
+}

packages/api/src/auth/middleware.ts

@@ -23,12 +23,13 @@ export type AuthDeps = {
   verifyToken?: TokenVerifier;
 };
 
-function roleFromClaims(claims: Record<string, unknown>, fallback: UserRole): UserRole {
+function roleFromClaims(claims: Record<string, unknown>): UserRole | undefined {
   const raw =
     (typeof claims["custom:role"] === "string" && claims["custom:role"]) ||
     (typeof claims.role === "string" && claims.role) ||
-    fallback;
-  return isUserRole(raw) ? raw : fallback;
+    undefined;
+  if (raw === undefined) return undefined;
+  return isUserRole(raw) ? raw : undefined;
 }
 
 function audienceMatches(payload: JWTPayload, expected: string): boolean {
@@ -141,7 +142,7 @@ export function createAuthMiddleware(env: ApiEnv, handle: Db, deps: AuthDeps = {
         cognitoSub: identity.sub,
         email: identity.email,
         name: identity.name,
-        role: roleFromClaims(payload as Record<string, unknown>, "viewer"),
+        role: roleFromClaims(payload as Record<string, unknown>),
       });
     } catch (error) {
       if (error instanceof IdentityConflictError) {

packages/api/src/auth/upsert-user.test.ts

@@ -5,7 +5,7 @@ import { IdentityConflictError, upsertUserFromIdentity } from "./upsert-user.js"
 function createDb(options: {
   bySub?: Record<string, unknown> | null;
   byEmail?: Record<string, unknown> | null;
-}): Db {
+}): { handle: Db; setSpy: ReturnType<typeof vi.fn> } {
   const findFirst = vi.fn(async (_args: { where: unknown }) => {
     // drizzle eq objects aren't introspectable here; alternate by call order.
     if (findFirst.mock.calls.length === 1) {
@@ -22,30 +22,35 @@ function createDb(options: {
     role: "admin" as const,
   };
 
+  const setSpy = vi.fn((patch: Record<string, unknown>) => ({
+    where: vi.fn(() => ({
+      returning: vi.fn(async () => [{ ...returningRow, ...patch, role: patch.role ?? returningRow.role }]),
+    })),
+  }));
+
   return {
-    driver: "postgres",
-    pool: { end: vi.fn(async () => undefined) } as never,
-    db: {
-      query: { users: { findFirst } },
-      update: vi.fn(() => ({
-        set: vi.fn(() => ({
-          where: vi.fn(() => ({
+    handle: {
+      driver: "postgres",
+      pool: { end: vi.fn(async () => undefined) } as never,
+      db: {
+        query: { users: { findFirst } },
+        update: vi.fn(() => ({
+          set: setSpy,
+        })),
+        insert: vi.fn(() => ({
+          values: vi.fn(() => ({
             returning: vi.fn(async () => [returningRow]),
           })),
         })),
-      })),
-      insert: vi.fn(() => ({
-        values: vi.fn(() => ({
-          returning: vi.fn(async () => [returningRow]),
-        })),
-      })),
-    } as never,
+      } as never,
+    },
+    setSpy,
   };
 }
 
 describe("upsertUserFromIdentity", () => {
   it("updates an existing row matched by cognito sub", async () => {
-    const handle = createDb({
+    const { handle } = createDb({
       bySub: {
         id: "11111111-1111-4111-8111-111111111111",
         cognitoSub: "seed-sub-admin",
@@ -66,8 +71,52 @@ describe("upsertUserFromIdentity", () => {
     expect(handle.db.update).toHaveBeenCalled();
   });
 
+  it("preserves the stored role when the token omits a role claim", async () => {
+    const { handle, setSpy } = createDb({
+      bySub: {
+        id: "11111111-1111-4111-8111-111111111111",
+        cognitoSub: "seed-sub-admin",
+        email: "admin@seahavenind.com",
+        name: "Dev Admin",
+        role: "admin",
+      },
+    });
+
+    const user = await upsertUserFromIdentity(handle, {
+      cognitoSub: "seed-sub-admin",
+      email: "admin@seahavenind.com",
+      name: "Dev Admin",
+    });
+
+    expect(setSpy).toHaveBeenCalledWith(
+      expect.not.objectContaining({ role: expect.anything() }),
+    );
+    expect(user.role).toBe("admin");
+  });
+
+  it("writes an explicit role claim over the stored role", async () => {
+    const { handle, setSpy } = createDb({
+      bySub: {
+        id: "11111111-1111-4111-8111-111111111111",
+        cognitoSub: "seed-sub-admin",
+        email: "admin@seahavenind.com",
+        name: "Dev Admin",
+        role: "viewer",
+      },
+    });
+
+    await upsertUserFromIdentity(handle, {
+      cognitoSub: "seed-sub-admin",
+      email: "admin@seahavenind.com",
+      name: "Dev Admin",
+      role: "approver",
+    });
+
+    expect(setSpy).toHaveBeenCalledWith(expect.objectContaining({ role: "approver" }));
+  });
+
   it("refuses to rebind an email owned by a different cognito sub", async () => {
-    const handle = createDb({
+    const { handle } = createDb({
       bySub: null,
       byEmail: {
         id: "11111111-1111-4111-8111-111111111111",

packages/api/src/auth/upsert-user.ts

@@ -7,7 +7,8 @@ export type AuthIdentity = {
   cognitoSub: string;
   email: string;
   name: string;
-  role: UserRole;
+  /** Present only when the token carries an explicit role claim. */
+  role?: UserRole;
 };
 
 export type AuthUser = {
@@ -46,6 +47,7 @@ function toAuthUser(row: {
 /**
  * Upsert by Cognito subject only. Never rebind an existing email to a new
  * subject — that would allow account takeover if email claims collide.
+ * Missing role claims leave the stored role unchanged.
  */
 export async function upsertUserFromIdentity(
   handle: Db,
@@ -56,14 +58,22 @@ export async function upsertUserFromIdentity(
   });
 
   if (bySub) {
+    const patch: {
+      email: string;
+      name: string;
+      role?: UserRole;
+      updatedAt: Date;
+    } = {
+      email: identity.email,
+      name: identity.name,
+      updatedAt: new Date(),
+    };
+    if (identity.role !== undefined) {
+      patch.role = identity.role;
+    }
     const [updated] = await handle.db
       .update(users)
-      .set({
-        email: identity.email,
-        name: identity.name,
-        role: identity.role,
-        updatedAt: new Date(),
-      })
+      .set(patch)
       .where(eq(users.id, bySub.id))
       .returning();
     return toAuthUser(updated);
@@ -83,7 +93,7 @@ export async function upsertUserFromIdentity(
       cognitoSub: identity.cognitoSub,
       email: identity.email,
       name: identity.name,
-      role: identity.role,
+      role: identity.role ?? "viewer",
     })
     .returning();
 

packages/api/src/db/client.ts

@@ -10,6 +10,17 @@ export type PostgresDb = ReturnType<typeof createPostgresDb>;
 export type DataApiDb = ReturnType<typeof createDataApiDb>;
 export type Db = PostgresDb | DataApiDb;
 
+type PostgresTx = Parameters<Parameters<PostgresDb["db"]["transaction"]>[0]>[0];
+type DataApiTx = Parameters<Parameters<DataApiDb["db"]["transaction"]>[0]>[0];
+
+/**
+ * Root connection or a transaction-scoped handle. Query helpers accept this so
+ * callers can pass `{ db: tx }` without casting a transaction to Db.
+ */
+export type DbHandle = {
+  db: Db["db"] | PostgresTx | DataApiTx;
+};
+
 function createPostgresDb(env: ApiEnv) {
   const pool = new pg.Pool({ connectionString: env.databaseUrl });
   return {

packages/api/src/http.ts

@@ -11,6 +11,17 @@ export type ErrorCode =
   | "INTERNAL_ERROR"
   | "DATABASE_UNAVAILABLE";
 
+export class ApiError extends Error {
+  constructor(
+    readonly status: ContentfulStatusCode,
+    readonly code: ErrorCode,
+    message: string,
+  ) {
+    super(message);
+    this.name = "ApiError";
+  }
+}
+
 export const CORRELATION_HEADER = "x-correlation-id";
 
 export type ErrorEnvelope = {

packages/api/src/routes/approvals.test.ts

@@ -3,7 +3,7 @@ import { createApp } from "../app.js";
 import { createMemoryDocumentsStore } from "../documents.js";
 import { loadEnv } from "../env.js";
 import type { ErrorEnvelope } from "../http.js";
-import { createFakeDb, SEED } from "../test/fake-db.js";
+import { createFakeDb, emptyStore, SEED } from "../test/fake-db.js";
 
 function expectEnvelope(body: unknown, code: string) {
   const envelope = body as ErrorEnvelope;
@@ -77,6 +77,56 @@ describe("approval stubs", () => {
     expectEnvelope(await response.json(), "FORBIDDEN");
   });
 
+  it("returns 403 when the caller is not the assignee for the step", async () => {
+    const store = emptyStore();
+    store.approvalSteps[0] = {
+      ...SEED.step,
+      assigneeUserId: "22222222-2222-4222-8222-222222222222",
+    };
+    const api = createApp(envFor("admin"), createFakeDb(store), {
+      documents: createMemoryDocumentsStore(),
+    });
+    const response = await api.request(`/api/approval-steps/${SEED.step.id}/decisions`, {
+      method: "POST",
+      headers: jsonHeaders,
+      body: JSON.stringify({ action: "approve" }),
+    });
+    expect(response.status).toBe(403);
+    expectEnvelope(await response.json(), "FORBIDDEN");
+    expect(store.approvalSteps[0]?.status).toBe("pending");
+  });
+
+  it("voids an invoice, closes pending steps, and rejects later decisions", async () => {
+    const store = emptyStore();
+    const api = createApp(envFor("admin"), createFakeDb(store), {
+      documents: createMemoryDocumentsStore(),
+    });
+    const voided = await api.request(`/api/invoices/${SEED.invoice.id}`, {
+      method: "PATCH",
+      headers: jsonHeaders,
+      body: JSON.stringify({ status: "void" }),
+    });
+    expect(voided.status).toBe(200);
+    await expect(voided.json()).resolves.toMatchObject({ status: "void" });
+    expect(store.approvalSteps[0]?.status).toBe("skipped");
+
+    const inbox = await api.request("/api/inbox");
+    expect(inbox.status).toBe(200);
+    const listed = (await inbox.json()) as { items: Array<{ id: string }> };
+    expect(listed.items.some((item) => item.id === SEED.step.id)).toBe(false);
+
+    // Re-open a pending step to prove voided invoices stay sealed even if a step remains.
+    store.approvalSteps[0] = { ...store.approvalSteps[0]!, status: "pending" };
+    const decision = await api.request(`/api/approval-steps/${SEED.step.id}/decisions`, {
+      method: "POST",
+      headers: jsonHeaders,
+      body: JSON.stringify({ action: "approve" }),
+    });
+    expect(decision.status).toBe(409);
+    expectEnvelope(await decision.json(), "CONFLICT");
+    expect(store.invoices[0]?.status).toBe("void");
+  });
+
   it("lists the caller inbox and accepts a comment", async () => {
     const api = app("admin");
     const inbox = await api.request("/api/inbox");

packages/api/src/routes/approvals.ts

@@ -169,12 +169,20 @@ export function createApprovalRoutes(handle: Db) {
     if (step.status !== "pending") {
       return errorJson(c, 409, "CONFLICT", "Step is not pending.");
     }
+    const user = caller(c);
+    if (!inboxVisible(step, user)) {
+      return errorJson(c, 403, "FORBIDDEN", "You are not an assignee for this approval step.");
+    }
+    const invoice = await firstById<InvoiceRow>(handle, invoices, step.invoiceId);
+    if (!invoice) return errorJson(c, 404, "NOT_FOUND", "Invoice not found.");
+    if (invoice.status === "void") {
+      return errorJson(c, 409, "CONFLICT", "Cannot act on a voided invoice.");
+    }
     const body = await parseJsonBody(c);
     const action = asString(body.action);
     if (!isDecision(action)) {
       return errorJson(c, 400, "VALIDATION_ERROR", "action must be approve, reject, or skip.");
     }
-    const user = caller(c);
     const nextStatus =
       action === "approve" ? "approved" : action === "reject" ? "rejected" : "skipped";
     const [updated] = await handle.db

packages/api/src/routes/helpers.ts

@@ -1,9 +1,9 @@
 import { randomUUID } from "node:crypto";
 import type { Context } from "hono";
-import type { Db } from "../db/client.js";
+import type { DbHandle } from "../db/client.js";
 import type { UserRole } from "../env.js";
 import { can, type RbacAction } from "../auth/rbac.js";
-import { errorJson } from "../http.js";
+import { ApiError, errorJson } from "../http.js";
 import type { AppBindings } from "../auth/middleware.js";
 
 const UUID_RE = /^[0-9a-f]{8}-[0-9a-f]{4}-[1-8][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i;
@@ -43,8 +43,12 @@ export function newId(): string {
   return randomUUID();
 }
 
-export function parseJsonBody(c: Context): Promise<Record<string, unknown>> {
-  return c.req.json<Record<string, unknown>>();
+export async function parseJsonBody(c: Context): Promise<Record<string, unknown>> {
+  try {
+    return await c.req.json<Record<string, unknown>>();
+  } catch {
+    throw new ApiError(400, "VALIDATION_ERROR", "Request body must be valid JSON.");
+  }
 }
 
 export function asMoney(value: unknown): string | null {
@@ -65,12 +69,12 @@ export function isDateOnly(value: string): boolean {
   return /^\d{4}-\d{2}-\d{2}$/.test(value);
 }
 
-export async function rowsOf<T>(handle: Db, table: unknown): Promise<T[]> {
+export async function rowsOf<T>(handle: DbHandle, table: unknown): Promise<T[]> {
   return (await handle.db.select().from(table as never)) as T[];
 }
 
 export async function firstById<T extends { id: string }>(
-  handle: Db,
+  handle: DbHandle,
   table: unknown,
   id: string,
 ): Promise<T | undefined> {

packages/api/src/routes/invoices.test.ts

@@ -168,6 +168,21 @@ describe("invoice stubs", () => {
     expectEnvelope(await response.json(), "VALIDATION_ERROR");
   });
 
+  it("returns 400 VALIDATION_ERROR for a malformed JSON body", async () => {
+    const app = createApp(envFor("admin"), createFakeDb(), {
+      documents: createMemoryDocumentsStore(),
+    });
+    const response = await app.request(`/api/invoices/${SEED.invoice.id}`, {
+      method: "PATCH",
+      headers: jsonHeaders,
+      body: "{not-json",
+    });
+    expect(response.status).toBe(400);
+    const body = (await response.json()) as ErrorEnvelope;
+    expect(body.error.code).toBe("VALIDATION_ERROR");
+    expect(body.error.message).toBe("Request body must be valid JSON.");
+  });
+
   it("keeps existing lines when a replace insert fails", async () => {
     const store = emptyStore();
     const handle = createFakeDb(store);

packages/api/src/routes/invoices.ts

@@ -1,11 +1,11 @@
 import { eq } from "drizzle-orm";
 import { Hono } from "hono";
-import type { Db } from "../db/client.js";
+import type { Db, DbHandle } from "../db/client.js";
 import type { DocumentsStore } from "../documents.js";
 import { documents, invoiceLines, invoices, vendors } from "../db/schema/index.js";
 import type { AppBindings } from "../auth/middleware.js";
 import { errorJson } from "../http.js";
-import { applyMatchingPolicy } from "../approvals.js";
+import { applyMatchingPolicy, closePendingSteps } from "../approvals.js";
 import {
   asMoney,
   asString,
@@ -103,13 +103,13 @@ function linesSumToAmount(lines: Array<{ amount: string }>, amount: string): boo
   return sum === moneyCents(amount);
 }
 
-async function linesFor(handle: Db, invoiceId: string): Promise<LineRow[]> {
+async function linesFor(handle: DbHandle, invoiceId: string): Promise<LineRow[]> {
   const rows = await rowsOf<LineRow>(handle, invoiceLines);
   return rows.filter((row) => row.invoiceId === invoiceId);
 }
 
 async function activeDuplicate(
-  handle: Db,
+  handle: DbHandle,
   vendorId: string,
   invoiceNumber: string,
   exceptId?: string,
@@ -214,7 +214,7 @@ export function createInvoiceRoutes(handle: Db, store: DocumentsStore) {
     let currentLines: LineRow[] = [];
     try {
       await handle.db.transaction(async (tx) => {
-        const scoped = { ...handle, db: tx } as typeof handle;
+        const scoped: DbHandle = { db: tx };
         const [row] = await tx
           .insert(invoices)
           .values({
@@ -335,6 +335,9 @@ export function createInvoiceRoutes(handle: Db, store: DocumentsStore) {
     }
     let row: InvoiceRow;
     try {
+      if (patch.status === "void") {
+        await closePendingSteps(handle, id, caller(c).id);
+      }
       [row] = await handle.db.update(invoices).set(patch).where(eq(invoices.id, id)).returning();
     } catch (error) {
       if (isUniqueViolation(error)) {