WFDASH-WEBAPP/docs/audit/backend-data.md

# Backend / API / Data Layer Audit

**Scope:** `src/lib/pocketbase.ts`, `src/lib/pb-helpers.ts` (legacy),
`src/lib/server/*-service.ts` (current layer), `src/app/api/v1/*` route
handlers, `/api/mcp`, and the route/service contract between them.

**Date:** 2026-08-28
**Author:** bob (audit only — no code changes shipped)

This audit is the data-layer companion to `security-perf-prod.md`
(tom, 2026-08-28). It deliberately does not re-cover the auth/secrets
surface — see that doc for the critical AGENT_API_KEY / PB admin / CORS
issues. Here we focus on:

- legacy `pb-helpers.ts` vs current `src/lib/server/*.service.ts` drift
- PB filter / query foot-guns (N+1, getFirstListItem, IN-vs-OR)
- schema-missing fallback pattern and select-field pitfalls
- input validation, error envelope, and route-handler boilerplate
- multi-write consistency (no transactions, partial-success semantics)
- service-layer API inconsistencies (admin PB vs cookie PB)

---

## 0. Headline summary

| Severity | Count | What it is |
| -------- | ----- | ---------- |
| Critical | 3     | Legacy `pb-helpers.ts` used directly by 32 files; schema-field gaps invisible to the type system; `pb-helpers.ts` PO/quote/order number generators are racy and were deprecated but still callable |
| Major    | 12    | No schema-validation library; 30+ `getFirstListItem` calls with inconsistent error handling; N+1 patterns in several services; `getAdminPocketBase` everywhere bypasses PB listRules; 7077-line MCP route file; schema-missing fallback duplicated 4× with subtle behavioural drift; cookie-auth PB singleton used by Server Components |
| Minor    | 10    | Inconsistent error envelopes; ad-hoc PO/quote-status string-literal duplication; search hard-caps hardcoded; legacy client-side duplicate-detection post-filter; no structured logging; service-layer returns vary between throwing and returning partial-success |

The legacy layer (`pb-helpers.ts`) is the single biggest source of
drift. AGENTS.md says "do NOT extend `pb-helpers.ts`", and the service
layer is the right place for new endpoints — but 32 callers still hit
it directly, and the data shape they consume is subtly different from
what the service layer emits. Migrating page-by-page is safe; the
hazard is doing it inconsistently.

---

## 1. Critical findings

### 1.1 `pb-helpers.ts` PO/quote/order number generators are racy, still exported

**File:** `src/lib/pb-helpers.ts:1755-1772` (`generateQuoteNumber`)
**File:** `src/lib/pb-helpers.ts:1773-1796` (`generateOrderNumber`)
**File:** `src/lib/pb-helpers.ts:1799-1864` (`generatePONumber`)

All three functions use the read-then-write pattern:

```ts
const existing = await pb.collection("orders").getList<Order>(1, 1, {
  filter: `order_number ~ "${prefix}"`,
  sort: "-order_number",
});
let seq = ... + 1;       // <- race window
return `ORD-${...}-${seq}`;
```

Two simultaneous calls both observe `007` and both write `008`. The
PB-side atomic counter collection (`order_counters`, `quote_counters`)
already exists and is used by `orders-service.ts:1302-1325` and
`quotes-service.ts:879-909`. AGENTS.md explicitly deprecates these
helpers, but they're still exported and reachable:

- `generateQuoteNumber` — `@deprecated` comment at line 1748, but
  nothing prevents a new caller from importing it.
- `generateOrderNumber` — no deprecation marker; no callers found
  via grep on the codebase, but still public.
- `generatePONumber` — inline comment at line 1745 calls it a
  "client-side mirror" of `src/lib/server/po-number.ts`. Both exist;
  the client-side one is racy.

**Recommended fix:** delete `generateQuoteNumber`, `generateOrderNumber`,
and `generatePONumber` from `pb-helpers.ts` (or move to a `legacy`
sub-export with a build-time ban) and add a CI lint rule that fails
if any non-`pb-helpers.ts` file imports them. Server-side callers
already use the atomic variants in `orders-service.ts` /
`quotes-service.ts` / `po-number.ts`.

### 1.2 Legacy `pb-helpers.ts` is the data layer for 32 files, drift risk

**File:** `src/lib/pb-helpers.ts:1-1994`
**File:** 32 files import from `@/lib/pb-helpers` (verified
2026-08-28 via `grep -rl 'from "@/lib/pb-helpers"' src/ | wc -l`)

AGENTS.md says new code goes through `src/lib/server/*.service.ts`.
The legacy helpers are still imported by Server Components,
Client Components, and form components directly. The two layers read
the same PB collections but emit subtly different shapes:

| Helper | Service-layer equivalent |
| ------ | ------------------------ |
| `getCustomer` / `getAllCustomers` (`pb-helpers.ts:32-46`) | `customers-service.ts` |
| `getCompanies` / `getCompany` / `getAllCompanies` (`pb-helpers.ts:48-67`) | not yet migrated |
| `getSuppliers` / `getSupplier` / `getAllSuppliers` (`pb-helpers.ts:69-87`) | not yet migrated |
| `getQuotes` (`pb-helpers.ts:213-247`) | `quotes-service.ts` |
| `getOrders` / `getOrder` / `getOrdersWithItems` (`pb-helpers.ts:617-635`, `:729-790`) | `orders-service.ts` |
| `createOrderFromQuote` (`pb-helpers.ts:1231...`) | `orders-service.createOrderFromQuote` |
| `getDeliveryNotes` (`pb-helpers.ts:1412-1420`) | `delivery-notes-service.ts` |

The risk is real and visible: `getQuotes` in `pb-helpers.ts:213` has
the `deliveryNoteOnly` option for the Quotes page delivery-note tab,
while `quotes-service.ts` re-implements the same query with a
different filter order. A page that switches from the helper to the
service changes the wire payload, which changes the page's render.

**Recommended fix:** per AGENTS.md, migrate page-by-page. The
highest-risk migrations first (orders, quotes) because they emit
shapes that already drift from the service. Add a `console.warn`
to the legacy helpers that fires once per session if `NODE_ENV !==
'test'` and the call comes from a Server Component — the breadcrumb
will surface un-migrated callers in the browser console.

### 1.3 `pocketbase.ts` browser singleton auth pattern doesn't translate to Server Components

**File:** `src/lib/pocketbase.ts:9-45`

```ts
let pb: PocketBase | null = null;
export function getPocketBase(): PocketBase {
  if (typeof window === "undefined") {
    // Server-side: create new instance each time
    return new PocketBase(POCKETBASE_URL);
  }
  // Client-side: reuse singleton — load auth from localStorage
  ...
}
```

Two problems:

1. The server-side path always creates an unauthenticated instance.
   Server Components that import `getPocketBase` make PB calls with
   the `_superusers` collection bypass or no auth at all, depending
   on PB rule configuration. `getPocketBase` itself can't impersonate
   the user — there's no cookie / header to read on the server. See
   `dev-identity.ts` for the project's solution (a cookie called
   `wfdash_dev_email` that holds the user's email as the dev
   placeholder).

2. The client-side singleton uses `localStorage` for the auth store
   (`pocketbase.ts:23-30`). This is fine for browser use, but it
   couples the data layer to the browser. Any attempt to share a
   helper between Server and Client Components requires a wrapper
   (the service layer does this via `getAdminPocketBase()`).

**Recommended fix:** make `getPocketBase` server-side UNSAFE (it
should throw if `typeof window === "undefined"`) and force all
server-side callers through `getAdminPocketBase`. Document the rule
in AGENTS.md. Today, server components using `getCustomer(id)` from
`pb-helpers.ts:32` are silently making unauthenticated PB calls —
PB's per-collection `listRule`/`viewRule` triages this, but a future
rule relaxation will silently expose data.

---

## 2. Major findings

### 2.1 No schema-validation library (no zod / yup / valibot / joi / ajv)

**File:** every `src/app/api/v1/**/route.ts` that calls `req.json()` (57 routes)

Input validation is entirely ad-hoc. Each route hand-rolls its
required-field checks:

```ts
// customers/route.ts:67-69
if (!body?.customer) {
  return NextResponse.json({ error: "customer is required" }, { status: 400 });
}
```

Verification: `grep -i 'zod\|yup\|valibot\|joi\|ajv' package.json`
returns 0 hits. The cost is real:

- **No type safety for body shapes** — TypeScript sees the body as
  `any` after `await req.json()`. The next line that reads
  `body.customer` will compile even if `customer` is missing.
- **Inconsistent error messages** — some routes say
  `"item_description is required."`, others say
  `"customer is required"`, others say
  `"Invalid JSON body"`. The MCP layer (`mcp/route.ts`) has a
  bespoke `QUOTE_ITEM_PATCH_FIELDS` allowlist (line 161-176), and
  the v1 layer has no equivalent on most endpoints.
- **No coercion** — string `"123"` and number `123` aren't
  normalised. Many service-layer functions wrap with `Number(...)`
  defensively (`pb-helpers.ts:1255-1262`), but a route handler that
  passes `body.cost_price` straight into a `pb.update(...)` will
  write a string into a number field.
- **No max-size enforcement** — the only size cap is the
  multipart file limit (25 MB on the `mcp.add_attachment` tool,
  not on JSON bodies). A 50 MB JSON body is parsed in full before
  the handler can 413.

**Recommended fix:** add `zod` to package.json (small, no deps,
TS-native), write one `parseBody(req, schema)` helper that lives in
`src/lib/server/http-helpers.ts`, and convert routes incrementally.
The 7 highest-value routes first (write endpoints that mutate PB):
customers, suppliers, companies, orders, order_items, quotes,
quote_items. The MCP route already enforces its own per-tool schema
via JSON Schema (not zod), so leave it alone.

### 2.2 `getFirstListItem` throws on miss — 30+ inconsistent handlers

**File:** 31 call sites in `src/lib/server/` and `src/app/api/v1/`

PB's `getFirstListItem(filter)` throws a 404 if the filter matches
zero rows. Half the call sites defensively catch, half don't. Examples:

- `orders-service.ts:1404` — `getFirstListItem(`year=${year} && month=${month}`)` is inside a try, catches via fallback (line 1411).
- `orders-service.ts:2574` — `getFirstListItem<Order>(`order_number = "${esc}"`)` is inside its own try/catch, returns `null` on 404 (line 2580).
- `management-service.ts:247` — `getFirstListItem(`company = "${escaped}"`)` returns the result (or rejects promise); the caller at line 245 wraps it in `.catch(() => null)` — but the path between line 240 and 247 doesn't catch.
- `management-service.ts:232-236` — two `.catch(() => null)` chains on `users` lookups, OK.

A non-defensive call site bubbles a 500 to the route handler, which
returns a generic `{error: message}` with the PB 404 stack trace
included. This is loud in logs but confusing to the operator — they
typed a real order number and got "404 not found" wrapped as a
500-class error.

**Recommended fix:** wrap once at the service boundary with a
`firstOrNull<T>(collection, filter)` helper. Every service should
call `firstOrNull` and let the route handler decide whether "no rows
matched" is a 404, 422, or a `null` field in a successful response.
This is the same pattern `findOrderByOrderNumber` uses at
`orders-service.ts:2573-2588` — apply it everywhere.

### 2.3 `getAdminPocketBase` everywhere bypasses PB listRules / field-level perms

**File:** every service-layer file (43 import sites)

The admin PB instance has superuser rights — every listRule, viewRule,
and modifyRule is bypassed. This means:

- **Data classification doesn't apply.** A future "sensitive
  customer" field added to `customers` with `viewRule: ""` (no
  public read) would still be readable through every v1 route, the
  MCP route, and the legacy helpers.
- **Field-level visibility doesn't apply.** Same shape: a
  `salary_band` field on `users` with a restrictive viewRule would
  be returned by every service that expands `users`.
- **Audit trail is lost.** Superuser actions log as
  `_superusers` in PB's audit log, not as the operator who
  actually made the call. The `getServerIdentity(req)` helper
  (`dev-identity.ts:42-58`) is used to record the operator on
  specific fields, but the PB audit row still says `_superusers`.

This is a known design decision, not a bug — the rationale (server
scripts, n8n, AI agents that can't carry the user's JWT) is documented
in `management-service.ts:1-25`. The risk is that the rationale
becomes invisible as more routes get added.

**Recommended fix:** add a comment to `pocketbase-admin.ts:62-72`
warning that the admin client bypasses ALL PB rules, and require a
JSDoc on every new service file that explains why admin is required.
For browser-facing reads (`customers/list/route.ts:30`), keep using
`assertBrowserSession` + the cookie PB (or migrate to the user-PB
instance). The `assertAgentAuth` / `assertBrowserSession` split is
correct as designed — the issue is when a *third* auth tier creeps
in (e.g. an `assertServiceAuth` for cron jobs) and the proof of
which tier a route uses gets buried in the route body.

### 2.4 N+1 patterns in service-layer loops

**File:** `src/lib/server/orders-service.ts:2174` (per-item patch loop inside standalone order create)
**File:** `src/lib/server/orders-service.ts:2503-2506` (`cancelOrder` per-item `item_status: "cancelled"` update loop)
**File:** `src/lib/server/quotes-service.ts` (multiple per-item update loops)
**File:** `src/lib/server/buyer-tasks-service.ts:115-167` (per-tab fetch × N tabs)

Examples:

```ts
// orders-service.ts:2174 (standalone order create flow)
for (let i = 0; i < itemsResult.created.length; i++) {
  const created = itemsResult.created[i];
  const original = input.items[i];
  if (!original) continue;
  const patch: Record<string, unknown> = {};
  if (original.sku !== undefined) patch.sku = original.sku;
  if (original.unit !== undefined) patch.unit = original.unit;
  if (original.supplier_id !== undefined) patch.supplier = original.supplier_id || null;
  if (original.notes !== undefined) patch.notes = original.notes;
  if (Object.keys(patch).length > 0) {
    try {
      await pb.collection(orderItemsCollection).update(created.id, patch);
    } catch (err) {
      // ... per-row failure recorded, loop continues
    }
  }
}
```

`createOrderItems` returns N rows; this loop fires N PB round-trips
to patch the secondary fields (sku, unit, supplier_id, notes). At
the WFDASH fleet scale (≤30 items per order) it's tolerable, but the
helper comment immediately above the loop admits this. Better
shape: pass the optional secondary fields into `createOrderItems` as
a parallel array, so the helper writes them in the same `create`
call. PB supports fields on create.

`cancelOrder` (`orders-service.ts:2503-2506`, the
`for (const item of toCancel) { await pb.collection(...).update(item.id, { item_status: "cancelled" }); }`
block) is the same shape: one PB write per cancelled item. The
surrounding `cancelOrdersBulk` (`orders-service.ts:2523-2535`) is a
*separate* loop at the order level — it iterates `orderIds` and
calls `cancelOrder` per order; the per-item fan-out lives inside
`cancelOrder` itself.

`buyer-tasks-service.ts:115` opens with
`Promise.all`-batched fetches for supplementary data, but the
surrounding loops are per-item on the response side.

**Recommended fix:** before each loop, ask "can I do this with one
PB call?" Specific wins:

- `orders-service.ts:2174` — fold secondary fields into the
  `createOrderItems` call so we don't need a follow-up patch.
- `cancelOrder` — collect all eligible items and use PB's batch
  create endpoint (or a single `update` with a different verb).
  PB 0.22+ supports transactional batch via `pb.send()`.
- `buyer-tasks-service.ts` — the per-tab fan-out is intentional
  and correct; the per-item post-processing inside each tab is the
  N+1 target.

### 2.5 No transactional writes across collections

**File:** every service that mutates >1 PB row in one operation

A representative example: `pb-helpers.ts:1264-1287`
(`createOrderFromQuote`) creates the `orders` row, then loops
`order_items` creates. If the network drops between the first
`order_items.create` and the last, the order exists with N items
where N is whatever the network reached. The transaction equivalent
would be PB's `pb.send()` with a chain of ops, or a manual
compensating-rollback helper.

The current mitigation is "log loudly and let the operator clean up
manually". This is fine at WFDASH fleet scale, but the pattern
scales badly — and the `OrderNumberCollisionError` flow at
`orders-service.ts:2145-2198` shows the cost of catching a partial-
failure state and rolling forward.

**Recommended fix:** two changes, in priority order:

1. Document the partial-success semantics in each multi-write
   service's JSDoc, with an explicit list of which steps are
   idempotent (safe to re-run) and which aren't. Today this
   knowledge lives in operator tribal memory.
2. For the highest-value multi-writes (order creation, PO creation,
   delivery note creation, quote split), add a `try/catch` that
   deletes / rolls-back the partial state on failure. The PB SDK's
   `pb.send()` batch endpoint supports this natively.

### 2.6 MCP route handler is a single 7077-line file

**File:** `src/app/api/mcp/route.ts:1-7077`

Everything — the CORS helper, JSON-RPC envelope, upstream-fetch
helper, schema validation, 50+ tool definitions, 50+ tool
handlers — lives in one file. Tool definitions alone occupy
lines 270-1500 (~1230 lines of `tools: [{ name: ..., inputSchema: ...}]`).
Tool handlers are interleaved with definitions rather than grouped,
so changing one tool requires scanning hundreds of lines.

This is a known single-file pattern (the existing audit covers it),
but the data-layer perspective adds:

- **No way to mock individual tools for tests.** The MCP route has
  tests at `route.test.ts` and `po-agent-workflow.test.ts`, but
  both test the route shape end-to-end. Service-layer splits would
  let each tool be tested in isolation (the underlying services
  already have unit tests).
- **Tool input schemas are written by hand** instead of using the
  same `zod` schema as the route handlers (see 2.1). The MCP layer
  could share schemas via `zod-to-json-schema` and the v1 routes.
- **Adding a new tool is a 30-minute scroll-and-edit.** A
  `tools/<name>.ts` per-tool structure would make `git diff` readable
  and reduce merge conflicts when multiple devs add tools.

**Recommended fix:** extract each tool's `inputSchema` and handler
into `src/lib/server/mcp-tools/<name>.ts`. The `tools` array becomes
`Object.entries(MCP_TOOLS).map(...)`. JSON-RPC dispatch stays in
`route.ts`. Risk: low — the file is a leaf in the import graph.

### 2.7 Schema-missing fallback pattern duplicated 4× with subtle drift

**File:** `src/lib/pb-helpers.ts:128-185` (`getQuotes`)
**File:** `src/lib/pb-helpers.ts:204-228` (`getQuoteAttachments`)
**File:** `src/lib/pb-helpers.ts:230-258` (`getQuoteStatusCounts`)
**File:** `src/lib/pb-helpers.ts:188-202` (`getTemplates` / `getFollowUpDueQuotes`)

All four use the same `is400(err)` + `warnSchemaMissing` pattern:

```ts
function is400(err: unknown): boolean { ... }
let warnedSchemaMissing = false;
function warnSchemaMissing(what: string) {
  if (warnedSchemaMissing) return;  // <- only warns ONCE for the whole process
  warnedSchemaMissing = true;
  console.warn(`[pb-helpers] ... (${what}). Import pb_schema to enable the new filter.`);
}
```

Three issues:

1. **`warnedSchemaMissing` is a module-level boolean**, so the warn
   fires once per process. After the first fallback, the operator
   never sees another "field missing" warning even if a different
   collection is missing a different field. The "what" argument is
   silently dropped.
2. **`is400` only checks `err.status === 400`**, but PB can also
   return 400 with a different error shape (the new PB versions
   embed a JSON `data` payload). The check misses anything that
   isn't a literal `{status: 400}` object.
3. **The fallback filters may compile but return wrong results.**
   `getQuotes` drops the `is_template != true` clause on fallback
   (line 175), so a quote marked as template silently appears in
   the New tab. `getQuoteStatusCounts` keeps the per-status queries
   but loses the templates-tab exclusion (line 263).

The 2026-07-22 PB schema import project (mentioned in AGENTS.md) is
the path that resolves this; until then, the fallbacks should at
minimum warn with the `what` argument.

**Recommended fix:** replace the module-level latch with a `Set<string>`
that tracks warned fields, and surface the warning to a UI
diagnostic (a banner on the operator dashboard, not a `console.warn`).
If a route's fallback returns wrong data, that should surface in
QA tests — write tests that assert "templates don't appear in the
New tab when `is_template` is missing".

### 2.8 Service-layer error envelopes vary between routes

**File:** every `src/app/api/v1/**/route.ts` that returns JSON

Three observed shapes:

```ts
// Shape A — most routes
{ error: "message" }

// Shape B — quote pdf route
{ error: "PDF render failed", detail: "..." }

// Shape C — quote pdf validation
{ error: "items not ready", code: "items_not_ready", details: { itemIds: [...], issues: [...] } }

// Shape D — MCP route (JSON-RPC)
{ jsonrpc: "2.0", id: ..., error: { code: -32010, message: "...", data: {...} } }
```

The route handler picks whichever envelope the original author
chose, with no consistent shape across endpoints. The MCP layer's
JSON-RPC envelope is correct per spec but uses error codes that
don't map to HTTP status (the agent has to learn the mapping from
the hint strings in `TOOL_HINTS` at `mcp/route.ts:271-450`).

The v1 route handlers also return inconsistent HTTP statuses for
similar failures:

- `customers/route.ts:78` returns 400 with `{error: "customer is required"}`.
- `customers/[id]/route.ts:48` returns 400 with `{error: "JSON object body required"}`.
- `quotes/[id]/pdf/route.ts:165-168` returns 422 with a structured body.
- `quote-items/route.ts` (need to read) returns 500 on validation failures (no dedicated check).

**Recommended fix:** standardise on:

```ts
{ error: { code: "string_enum", message: "human-readable", details?: object } }
```

with a top-level helper `err(code, message, status, details?)`. The
MCP layer keeps its JSON-RPC envelope (it's a different protocol)
but maps to the same codes internally.

### 2.9 Search hard-caps silently truncate results

**File:** `src/lib/pb-helpers.ts:1066` (`SEARCH_HARD_CAP = 500`)
**File:** `src/lib/server/orders-service.ts:1075` (search caps in service layer, separate value)
**File:** `src/lib/pb-helpers.ts:431-453` (status counts — 22 parallel queries)

`SEARCH_HARD_CAP = 500` caps `searchOrdersWithItems` at 500 results.
Past 500, the result is silently truncated — no warning to the
operator that there are more matches.

The status counts (`getQuoteStatusCounts` at `pb-helpers.ts:230`)
fires 22 parallel `getList(1, 1, ...)` queries, one per PB status.
With no `quotes.status` index on the live PB, each query is a full
scan over ~8.5k quotes = ~2.6s wall time per the inline comment.
The service-layer replacement (`quotes-service.ts`) collapses this
to 1 scan.

**Recommended fix:** when a search hits the hard cap, return
`{ items, truncated: true, totalApprox: "≥ 500" }` so the UI can
render "showing 500 of 500+, refine your query". Migrate the status
counts through the service layer's collapsed-counts implementation.

### 2.10 `pb-helpers.ts` duplicate-detection post-filter is fine but verbose

**File:** `src/lib/pb-helpers.ts:1889-1953` (`findNameMatches`, `findCompanyDuplicates`, etc.)

PB's `~` operator does case-insensitive substring match; for exact
case-insensitive equality, the helper does:

```ts
const result = await pb.collection(collectionName).getList<T>(1, limit, {
  filter: `${field} ~ "${escapeDup(trimmed)}"`,
});
return result.items.filter((row) => normDup(row[field]) === target);
```

This works at WFDASH scale but has two limitations:

- `(field) ~ "value"` is still substring, so a search for "Acme"
  returns "Acme Limited" AND "New Acme Holdings" both. The
  post-filter narrows to exact, but the wire payload already
  included the unwanted rows.
- The helper calls `pb.collection(...).getList` rather than the
  service-layer wrapper, so the auth context drifts (cookie PB vs
  admin PB depending on caller).

**Recommended fix:** migrate `findCompanyDuplicates` /
`findSupplierDuplicates` / `findCustomerDuplicates` into
`management-service.ts` so they use admin PB (consistent with the
CRUD paths), and use PB's `=` with a `lower()` equivalent. PB 0.26
doesn't have `lower()`, so the post-filter is unavoidable, but the
narrowed PB `filter` should be the strict substring with `~`, not
a substring that includes everything.

### 2.11 Counter `last_seq+` increments — concurrent-edge case

**File:** `src/lib/server/orders-service.ts:1413-1426` (`getOrCreateOrderCounter`)
**File:** `src/lib/server/quotes-service.ts:930-944` (mirror for quotes)

```ts
try {
  return await pb.collection(ORDER_COUNTERS_COLLECTION)
    .getFirstListItem(`year=${year} && month=${month}`);
} catch {
  // First request for this (year, month) — backfill from existing orders,
  // then create. Without backfill the new code would start at ORD-...-001
  // and collide with the existing sequence.
  const backfill = await getMaxOrderSequenceInMonth(pb, year, month);
  try {
    return await pb.collection(ORDER_COUNTERS_COLLECTION).create({
      year, month, last_seq: backfill,
    });
  } catch {
    // Concurrent caller beat us to it — re-fetch.
    return await pb.collection(ORDER_COUNTERS_COLLECTION)
      .getFirstListItem(`year=${year} && month=${month}`);
  }
}
```

The third `try` (line 1421-1425) catches the concurrent-beat-us case
but doesn't catch the underlying unique-constraint error inside the
`.catch`. If the unique constraint fails AND the re-fetch also fails,
the original error is swallowed.

More importantly: `getMaxOrderSequenceInMonth` does a
`getFullList({ filter: order_number ~ "..." })`, which scans every
order in the month. On a busy month (or for the orders of the
month, not just the current one), this is the slow path of an
otherwise O(1) counter increment.

**Recommended fix:** the third try should re-throw the original
error if the re-fetch fails. The slow-path backfill can be a one-
shot job run by ops, not a per-create step.

### 2.12 PO number generator `getOrCreate` lookup lacks fallback for race

**File:** `src/lib/server/po-number.ts:1-...` (mirror of `pb-helpers.ts:1799-1864`)

The server-side PO number generator lives at `src/lib/server/po-number.ts`
(matching the deprecated client-side `generatePONumber` at
`pb-helpers.ts:1799-1864`). Per the inline comment at
`pb-helpers.ts:1745-1752`, they should mirror field-by-field.

Two issues:

1. The client-side helper at `pb-helpers.ts:1799-1864` post-filters
   the existing POs on the suffix (line 1840-1848). The server-side
   helper (read separately) presumably uses a more correct index hit.
   Verify the formats actually match — a divergence here means
   previews in the email composer don't match the persisted PO
   number.

2. Both helpers look up "all POs in the (year, month)" via
   `getList(1, 500)` and filter client-side. For a busy month this
   pulls 500 records per PO generation. At the WFDASH fleet scale
   it's tolerable but wasteful.

**Recommended fix:** read both side-by-side and assert the formats
match byte-for-byte. Add a comment in `po-number.ts` pinning the
contract: "client-side preview at `pb-helpers.ts:1799-1864` MUST
produce the same string".

---

## 3. Minor findings

### 3.1 `deliveryNoteOnly` option on `getQuotes` is fragile

**File:** `src/lib/pb-helpers.ts:213-247` (`getQuotes` options parameter)

The `options: { deliveryNoteOnly?: boolean }` shape is the only
optional options object on the legacy helper. Adding a second
boolean later would require a breaking change to the type. Use a
discriminated options object instead — `getQuotes(page, perPage, search, sort, statuses, options)`.

### 3.2 Cookie PB singleton persists auth across logout

**File:** `src/lib/pocketbase.ts:23-46`

When the user clicks logout, the operator's PB token is removed
from `localStorage` (presumably — verify in the logout UI), but
the singleton `pb` instance in `pocketbase.ts:9` is module-scoped
and not cleared. The next `getPocketBase()` returns the same
instance with the stale auth.

**Recommended fix:** add an exported `clearPocketBaseAuth()` that
resets the singleton and clears the localStorage entry. Call it
from the logout handler.

### 3.3 `assertBrowserSession` is duplicated inline in `quotes/items/rewrite/route.ts`

**File:** `src/app/api/v1/quotes/items/rewrite/route.ts:99-122`
**File:** `src/lib/server/browser-session.ts:1-30`

The rewrite route has its own `assertBrowserSession` instead of
importing the shared one. The two implementations are identical
(`fetch ${pbUrl}/api/collections/users/auth-refresh` with a 5s
timeout). Drift risk if one is updated and the other isn't.

**Recommended fix:** import from `@/lib/server/browser-session`.

### 3.4 `isPocketBaseId` regex is too lax (downgraded — minor)

**File:** `src/lib/server/management-service.ts:253-257`

```ts
function isPocketBaseId(s: string): boolean {
  // PocketBase ids are 15 alphanumeric chars by default. We don't try
  // to be stricter than that — false positives just fall through to
  // the name lookup which is harmless.
  return /^[a-z0-9]{15}$/i.test(s);
}
```

PB's default id format is 15 alphanumeric chars, but admins can
configure custom id formats. The regex matches any 15-char
alphanumeric string (e.g. `"customerCustomer1234"` — not a real PB
id). However, the actual call sites (`resolveCompanyRef` /
`resolveUserRef` at `management-service.ts:206-237`) wrap the
`getOne` in `try/catch` and **explicitly fall through to the name
lookup** on any failure (literal comment: `// Fall through to name
lookup.` / `// Fall through.`). So a 15-char string that doesn't
resolve as an id correctly triggers the name-lookup fallback, and
a string that *does* resolve as an id returns the real record.

The remaining (narrow) risk is a false positive that happens to
collide with an unrelated record's real 15-char PB id — i.e. the
operator types a company name like `"abc123def456ghi"` and it
silently shadows a different real company. Probability is low
(15-char alphanumeric is a ~10^27 space at 15 chars), but not
zero, and the symptom is silent mis-routing. This is **minor**, not
major.

**Severity:** Minor (downgraded from a prior draft that incorrectly
described the fallback as missing — the fallback IS present).

**Recommended fix:** tighten the regex when PB exposes a stable id
shape (e.g. include the `pre_` / custom-prefix configuration), or
require a prefix hint on user-supplied input. For the time being,
add a unit test that asserts both the fast-path and the fallback
paths resolve to the same id when given an ambiguous 15-char input.

### 3.5 PB file proxy is publicly readable (per code comment)

**File:** `src/app/api/v1/quotes/[id]/pdf/route.ts:694-697`

```ts
// PB's `/api/files/...` endpoint is publicly readable for these
// records (no Authorization header required — the file collection
// rule allows it). Stream the bytes through so the iframe can
// render without exposing the storage URL directly.
```

The comment admits that PB's per-file rule allows anonymous reads.
The PDF route streams the bytes back through Next so the iframe
gets the data via the same-origin response, but anyone with the
file token can hit `${PB}/api/files/quote_attachments/${id}/${name}`
directly. The `security-perf-prod.md` audit covers this.

**Recommended fix (data-layer specific):** add a PB rule that gates
`/api/files/quote_attachments/*` on a valid auth token. Today,
the application relies on the file token being unguessable — PB's
token format is randomsuffixed so it's hard to enumerate, but the
attachment-id IS in the URL.

### 3.6 Schema-mismatch silently degrades `quote_attachments` direction

**File:** `src/lib/pb-helpers.ts:540-585` (`getQuoteAttachments`)

The `direction` field on `quote_attachments` was added in a 2026-06-23
migration. The fallback at `pb-helpers.ts:570-583` returns the
historical "all attachments" view when the field doesn't exist.
This means after the migration, an operator who hasn't run it sees
all attachments (including outbound and generated), which changes
the "Original Email" card on the quote detail page. Document this
in the migration guide.

### 3.7 `csv` cell encoding is naive

**File:** `src/lib/pb-helpers.ts:1914-1929` (`exportToCSV`)

The CSV encoder only escapes commas and double quotes, not
newlines. A description with `\n` (rare in the codebase, but
possible from copy/paste from a Word doc) breaks the CSV row.

**Recommended fix:** also escape `\n` and `\r` (CRLF inside a quoted
cell is allowed by RFC 4180, but only if the cell is quoted; the
current code only quotes when commas or double quotes are present).

### 3.8 `console.error` count is 37 across 165+ routes

**File:** `console.error` and `console.warn` callsites across `src/app/api/v1/**`

A `grep -c 'console.error\|console.warn\|console.log' src/app/api/v1/**/*.ts` shows 37. Most routes either log nothing or log a single error per failure. With structured logging missing (per the existing audit's 2.8), debugging a customer-reported failure today requires PB logs + Next logs + guessing.

**Recommended fix:** introduce a `logError(scope, error, context)` helper that adds the route path, request id, and container id to a single JSON line. Wrap every `catch` block with it. Even without a logger backend, the JSON-line format makes production logs grep-able.

### 3.9 `searchOrdersWithItems` SEARCH_HARD_CAP is in legacy only

**File:** `src/lib/pb-helpers.ts:1066` vs `src/lib/server/orders-service.ts:1075`

The legacy cap is 500; the service-layer cap is presumably different
(verify). Different caps means a page that migrated to the service
shows a different count than a page that hasn't.

### 3.10 `pb-helpers.ts` `getOrderItemStatusTabCounts` is N round-trips

**File:** `src/lib/pb-helpers.ts:660-693`

Six `getFullList` calls in parallel (`tabs.map(...)`). Each scans
`order_items` filtered by `item_status` — at WFDASH scale this is
fast, but each tab also returns the full `order` relation string,
even though the function only dedupes to a count. Add `fields: "order"`
to narrow the wire payload (this is the same fix the service layer
applies).

---

## 4. Production-readiness checklist (data-layer specific)

- [ ] **Schema validation:** add `zod`, write `parseBody(req, schema)` helper, convert the 7 highest-risk write routes.
- [ ] **Legacy deprecation:** move `generateQuoteNumber`, `generateOrderNumber`, `generatePONumber` out of `pb-helpers.ts` exports; add a lint rule that bans non-legacy imports.
- [ ] **Browser-singleton fix:** `getPocketBase` throws on server-side. Service-layer is the only path.
- [ ] **getFirstListItem abstraction:** write `firstOrNull<T>(collection, filter)` and migrate the 31 call sites.
- [ ] **Multi-write rollback:** document partial-success semantics on the 4 multi-write services (orders, PO, DN, quote-split).
- [ ] **MCP route refactor:** extract each tool into `lib/server/mcp-tools/<name>.ts`.
- [ ] **Search cap visibility:** return `truncated: true` from any search that hits a hard cap.
- [ ] **Schema-missing warnings:** `Set<string>` per-field, surfaced to a UI diagnostic.
- [ ] **Error envelope standardisation:** `{ error: { code, message, details? } }` shape across all v1 routes.
- [ ] **`isPocketBaseId` tightening:** verify both id and name lookups for ambiguous inputs.
- [ ] **CSV export hardening:** escape `\n` and `\r`.
- [ ] **Logging:** structured JSON `logError(scope, error, context)` helper.

## 5. What I deliberately did **not** flag

- **Auth surface.** Covered comprehensively in `security-perf-prod.md`
  §1.1-1.4, §2.1-2.2, §2.11. This audit stays data-focused.
- **Frontend / UX issues.** See `frontend-ux.md`.
- **PB schema field disuse.** See `unused-pb-fields.md`.
- **Test coverage gaps.** The service-layer files have *.test.ts
  companions for every business-critical module; the gaps are at
  the route-handler layer (most routes have no tests, e.g.
  `agent/scan/route.ts`). Coverage audit is a separate task.
- **PDF rendering / @react-pdf / SVG data URIs.** The PDF rendering
  foot-guns (raster corruption at byte 0x80) are covered in the
  PDF route JSDoc and don't affect the data layer.