fix(sales): route TrackShip delivery poll through status-transition path (closes audit-C2) (#193)

* fix(sales): route TrackShip delivery poll through the status-transition path

Audit finding C2 (onetwo3d-ims-erl0): checkDeliveryStatus marked orders
DELIVERED via a raw db.salesOrder.update — no transition guard, no
side effects. A delivery detected by the cron skipped everything the
manual DELIVERED path does (status_changed activity log, WooCommerce
status push, cache revalidation), and could overwrite an order that had
moved to CANCELLED/REFUNDED between the SHIPPED query and the write.

Now routed through applySalesOrderStatusTransition with the internal
bypass token (cron has no session, so the permission check is skipped
but the guard + side effects run). The state machine rejects
DELIVERED from a non-deliverable status, so a since-cancelled order is
no longer silently overwritten — the cron logs delivery_status_skipped
and moves on.

Extracted the delivery-marking into markOrderDelivered with injected
transition + log deps so the routing and skip-on-reject behaviour are
unit-tested without the DB or external API (2 tests). workflows.md
documents that cron-driven DELIVERED runs the same guard + side
effects as manual.

Closes onetwo3d-ims-erl0.

* fix(sales): TrackShip delivery uses skipPermissionCheck, keeping the state guard (review hardening)

Codex adversarial pass found the original C2 fix did NOT actually
protect against the stale-state race it claimed to. The internal bypass
token makes validateManualSalesOrderStatusTransition short-circuit
{ success: true }, skipping BOTH the permission check AND the
state-machine guard — so a SHIPPED→CANCELLED race between the poll's
query and the write would still overwrite CANCELLED with DELIVERED, and
the docs claim was false.

Root fix: added a narrower skipPermissionCheck option to
applySalesOrderStatusTransition that skips ONLY the permission check;
the state-machine guard (pre-lock AND under-lock via
updateSalesOrderStatusUnderLock) still runs. The WooCommerce status-sync
path keeps the bypass token (skip-both) unchanged — it legitimately
forces mapped statuses and already handles rejection. TrackShip now uses
skipPermissionCheck, so a since-cancelled/refunded order is genuinely
rejected under the lock and logged as delivery_status_skipped.

Also from the review:
- Hoisted the applySalesOrderStatusTransition dynamic import above the
  per-order loop (was re-evaluated each delivered order).
- Wrapped the per-order mark in try/catch so one order's failure (e.g.
  a transient log write) no longer aborts the whole cron batch.
- Dropped the  cast in the test; deps.transition type now
  matches the real signature (extra/options optional), so a future
  signature drift is caught.

Docs corrected: the cron skips permission only, not the state guard.
2/2 helper tests, 93/93 sales-domain tests.

---------

Co-authored-by: OneTwo3D IMS <dev@onetwo3d.com>

Author: OneTwo3D

app/actions/sales.ts

@@ -1224,11 +1224,17 @@ export async function applySalesOrderStatusTransition(
   id: string,
   targetStatus: SoStatus,
   extra?: { trackingNumber?: string; shipFromWarehouseId?: string },
-  options?: { pushStatusToWooCommerce?: boolean; internalBypassToken?: symbol },
+  options?: { pushStatusToWooCommerce?: boolean; internalBypassToken?: symbol; skipPermissionCheck?: boolean },
 ): Promise<{ success: boolean; error?: string }> {
   try {
+    // internalBypassToken skips BOTH the permission check and the state-machine
+    // guard — used by external systems (WooCommerce) that may legitimately force
+    // a mapped status. skipPermissionCheck is narrower: it skips ONLY the
+    // permission check (for sessionless internal callers such as the delivery
+    // cron) while the state-machine guard still runs, so a stale transition
+    // (e.g. an order cancelled after the poll's SHIPPED query) is still rejected.
     const bypassPermission = options?.internalBypassToken === INTERNAL_STATUS_TRANSITION_BYPASS
-    if (!bypassPermission) {
+    if (!bypassPermission && !options?.skipPermissionCheck) {
       await requirePermission('sales.process')
     }
     const so = await db.salesOrder.findUnique({

docs/workflows.md

@@ -31,6 +31,16 @@ rules.
 | `REFUNDED` | None | Terminal. |
 | `SHIPPED` | `COMPLETED`, `DELIVERED`, `PARTIALLY_REFUNDED`, `REFUNDED` | - |
 
+The `DELIVERED` transition can be driven automatically by delivery-tracking
+polling (TrackShip or the active shopping connector, via `/api/cron/delivery-status`).
+That cron path runs the **same** transition guard and side effects as a manual
+delivery — it does not write the status directly. Because the cron has no user
+session it skips only the permission check (not the state-machine guard, unlike
+the WooCommerce status-sync path), so if an order has moved out of a deliverable
+state (e.g. cancelled or refunded after dispatch) between the poll's SHIPPED
+query and the under-lock write, the guard rejects the change and the cron logs a
+`delivery_status_skipped` warning instead of forcing it.
+
 ### Shipments
 
 | Status | Allowed next statuses | Notes |

lib/trackship.ts

@@ -71,6 +71,70 @@ export async function queryTrackShip(apiKey: string, trackingNumber: string, car
   }
 }
 
+// ---------------------------------------------------------------------------
+// Mark a delivered order
+// ---------------------------------------------------------------------------
+
+type DeliveredTarget = { id: string; externalOrderNumber: string | null; source: string }
+type MarkDeliveredDeps = {
+  transition: (
+    id: string,
+    targetStatus: 'DELIVERED',
+    extra?: { trackingNumber?: string; shipFromWarehouseId?: string },
+    options?: { skipPermissionCheck?: boolean },
+  ) => Promise<{ success: boolean; error?: string }>
+  log: typeof logActivity
+}
+
+/**
+ * Mark an order DELIVERED from a delivery-tracking poll.
+ *
+ * Routes through the real status-transition path rather than a raw status
+ * write (audit C2): the transition re-validates the state machine under a lock
+ * — so an order that moved to CANCELLED / REFUNDED / etc. between the SHIPPED
+ * query and now is rejected, not silently overwritten — and runs the same side
+ * effects a manual DELIVERED transition does (status_changed log, WooCommerce
+ * status push, cache revalidation). The cron has no session, so it passes
+ * skipPermissionCheck to skip ONLY the sales.process permission check; the
+ * state-machine guard still runs (unlike the WooCommerce bypass token, which
+ * skips both). Dependency-injected so the routing + skip-on-reject behaviour
+ * is unit-testable without the DB or external API.
+ */
+export async function markOrderDelivered(
+  target: DeliveredTarget,
+  deps: MarkDeliveredDeps,
+): Promise<{ delivered: boolean }> {
+  const result = await deps.transition(target.id, 'DELIVERED', undefined, {
+    skipPermissionCheck: true,
+  })
+  if (result.success) {
+    await deps.log({
+      entityType: 'SALES_ORDER',
+      entityId: target.id,
+      action: 'delivered',
+      tag: 'sales',
+      level: 'INFO',
+      description: `Order ${target.externalOrderNumber} marked as delivered via ${target.source}`,
+      metadata: { orderNumber: target.externalOrderNumber, source: target.source },
+      resolveUser: false,
+    })
+    return { delivered: true }
+  }
+  // No longer in a state that can transition to DELIVERED (e.g. cancelled or
+  // refunded after dispatch). Log and skip rather than forcing it.
+  await deps.log({
+    entityType: 'SALES_ORDER',
+    entityId: target.id,
+    action: 'delivery_status_skipped',
+    tag: 'sales',
+    level: 'WARNING',
+    description: `Skipped marking order ${target.externalOrderNumber} delivered via ${target.source}: ${result.error ?? 'transition rejected'}`,
+    metadata: { orderNumber: target.externalOrderNumber, source: target.source, error: result.error ?? null },
+    resolveUser: false,
+  })
+  return { delivered: false }
+}
+
 // Check delivery status for all shipped orders
 // ---------------------------------------------------------------------------
 
@@ -106,6 +170,9 @@ export async function checkDeliveryStatus(): Promise<{ checked: number; delivere
   let checked = 0
   let delivered = 0
 
+  // Hoisted: the transition module is loaded once, not per delivered order.
+  const { applySalesOrderStatusTransition } = await import('@/app/actions/sales')
+
   for (const order of shippedOrders) {
     let isDelivered = false
 
@@ -135,22 +202,18 @@ export async function checkDeliveryStatus(): Promise<{ checked: number; delivere
     }
 
     if (isDelivered) {
-      await db.salesOrder.update({
-        where: { id: order.id },
-        data: { status: 'DELIVERED' },
-      })
-      delivered++
       const resolvedSource = useShoppingConnector ? 'shopping_connector' : 'trackship'
-      await logActivity({
-        entityType: 'SALES_ORDER',
-        entityId: order.id,
-        action: 'delivered',
-        tag: 'sales',
-        level: 'INFO',
-        description: `Order ${order.externalOrderNumber} marked as delivered via ${resolvedSource}`,
-        metadata: { orderNumber: order.externalOrderNumber, source: resolvedSource },
-        resolveUser: false,
-      })
+      try {
+        const result = await markOrderDelivered(
+          { id: order.id, externalOrderNumber: order.externalOrderNumber, source: resolvedSource },
+          { transition: applySalesOrderStatusTransition, log: logActivity },
+        )
+        if (result.delivered) delivered++
+      } catch (error) {
+        // Isolate per-order failures (e.g. a transient DB error while logging)
+        // so one bad order does not abort delivery processing for the rest.
+        console.error(`[delivery-status] failed to mark order ${order.id} delivered:`, error)
+      }
     }
   }
 

tests/trackship-mark-delivered.test.ts

@@ -0,0 +1,61 @@
+import assert from 'node:assert/strict'
+import { test } from 'node:test'
+import { markOrderDelivered } from '@/lib/trackship'
+
+type TransitionCall = {
+  id: string
+  targetStatus: string
+  extra?: { trackingNumber?: string; shipFromWarehouseId?: string }
+  options?: { skipPermissionCheck?: boolean }
+}
+
+function makeDeps(transitionResult: { success: boolean; error?: string }) {
+  const calls: { transition: TransitionCall[]; logs: Array<Record<string, unknown>> } = { transition: [], logs: [] }
+  const deps = {
+    transition: async (
+      id: string,
+      targetStatus: 'DELIVERED',
+      extra?: { trackingNumber?: string; shipFromWarehouseId?: string },
+      options?: { skipPermissionCheck?: boolean },
+    ) => {
+      calls.transition.push({ id, targetStatus, extra, options })
+      return transitionResult
+    },
+    log: async (entry: Record<string, unknown>) => { calls.logs.push(entry) },
+  }
+  return { deps, calls }
+}
+
+test('routes delivery through the transition path skipping only permission (not a raw write)', async () => {
+  const { deps, calls } = makeDeps({ success: true })
+  const result = await markOrderDelivered(
+    { id: 'so-1', externalOrderNumber: 'WC-100', source: 'trackship' },
+    deps,
+  )
+  assert.equal(result.delivered, true)
+  assert.equal(calls.transition.length, 1)
+  const call = calls.transition[0]
+  assert.equal(call.id, 'so-1')
+  assert.equal(call.targetStatus, 'DELIVERED')
+  // skipPermissionCheck skips only the permission gate — the state-machine
+  // guard still runs, so a since-cancelled order is rejected (success:false).
+  assert.equal(call.options?.skipPermissionCheck, true)
+  // Logs the successful delivery.
+  assert.equal(calls.logs.length, 1)
+  assert.equal(calls.logs[0].action, 'delivered')
+})
+
+test('skips and warns when the transition is rejected (order no longer SHIPPED)', async () => {
+  const { deps, calls } = makeDeps({ success: false, error: 'Cannot move from CANCELLED to DELIVERED' })
+  const result = await markOrderDelivered(
+    { id: 'so-2', externalOrderNumber: 'WC-200', source: 'shopping_connector' },
+    deps,
+  )
+  // Not counted as delivered, and no forced write — only a warning log.
+  assert.equal(result.delivered, false)
+  assert.equal(calls.transition.length, 1)
+  assert.equal(calls.logs.length, 1)
+  assert.equal(calls.logs[0].action, 'delivery_status_skipped')
+  assert.equal(calls.logs[0].level, 'WARNING')
+  assert.match(String(calls.logs[0].description), /Cannot move from CANCELLED to DELIVERED/)
+})