From de7d5354748b3ec61cfc5ccb0604f0ffc278a406 Mon Sep 17 00:00:00 2001 From: OneTwo3D IMS Date: Sat, 13 Jun 2026 13:15:02 +0000 Subject: [PATCH 1/2] fix(sales): route TrackShip delivery poll through the status-transition path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- docs/workflows.md | 8 +++ lib/trackship.ts | 85 +++++++++++++++++++++----- tests/trackship-mark-delivered.test.ts | 51 ++++++++++++++++ 3 files changed, 129 insertions(+), 15 deletions(-) create mode 100644 tests/trackship-mark-delivered.test.ts diff --git a/docs/workflows.md b/docs/workflows.md index 9cf5ac81..f5cb94c3 100644 --- a/docs/workflows.md +++ b/docs/workflows.md @@ -31,6 +31,14 @@ 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. 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 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 | diff --git a/lib/trackship.ts b/lib/trackship.ts index 779372bd..fcd78609 100644 --- a/lib/trackship.ts +++ b/lib/trackship.ts @@ -9,6 +9,7 @@ import { db } from '@/lib/db' import { logActivity } from '@/lib/activity-log' import { getOrderDeliveryStatus } from '@/lib/shopping' +import { INTERNAL_STATUS_TRANSITION_BYPASS } from '@/lib/sales/status-transition-bypass' // --------------------------------------------------------------------------- // TrackShip API (direct) @@ -71,6 +72,69 @@ 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: undefined, + options: { internalBypassToken: symbol }, + ) => 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 under a lock — so an order + * that moved to CANCELLED / REFUNDED / etc. between the SHIPPED query and now + * is 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 the internal + * bypass token to skip the sales.process permission check while keeping the + * guard and side effects. 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, { + internalBypassToken: INTERNAL_STATUS_TRANSITION_BYPASS, + }) + 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 // --------------------------------------------------------------------------- @@ -135,22 +199,13 @@ 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, - }) + const { applySalesOrderStatusTransition } = await import('@/app/actions/sales') + const result = await markOrderDelivered( + { id: order.id, externalOrderNumber: order.externalOrderNumber, source: resolvedSource }, + { transition: applySalesOrderStatusTransition, log: logActivity }, + ) + if (result.delivered) delivered++ } } diff --git a/tests/trackship-mark-delivered.test.ts b/tests/trackship-mark-delivered.test.ts new file mode 100644 index 00000000..fa585e9c --- /dev/null +++ b/tests/trackship-mark-delivered.test.ts @@ -0,0 +1,51 @@ +import assert from 'node:assert/strict' +import { test } from 'node:test' +import { markOrderDelivered } from '@/lib/trackship' + +const INTERNAL_STATUS_TRANSITION_BYPASS_DESC = 'internal-status-transition-bypass' + +function makeDeps(transitionResult: { success: boolean; error?: string }) { + const calls: { transition: unknown[]; logs: Array> } = { transition: [], logs: [] } + const deps = { + transition: async (id: string, targetStatus: 'DELIVERED', extra: undefined, options: { internalBypassToken: symbol }) => { + calls.transition.push({ id, targetStatus, extra, options }) + return transitionResult + }, + log: async (entry: Record) => { calls.logs.push(entry) }, + } + return { deps: deps as never, calls } +} + +test('routes delivery through the transition path with the bypass token (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] as { id: string; targetStatus: string; options: { internalBypassToken: symbol } } + assert.equal(call.id, 'so-1') + assert.equal(call.targetStatus, 'DELIVERED') + // The bypass token is the symbol that lets the cron skip the permission check. + assert.equal(typeof call.options.internalBypassToken, 'symbol') + assert.equal(call.options.internalBypassToken.description, INTERNAL_STATUS_TRANSITION_BYPASS_DESC) + // 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/) +}) From 39d9c9b0a0aeb59ee1ac59f400135acd74742a36 Mon Sep 17 00:00:00 2001 From: OneTwo3D IMS Date: Sat, 13 Jun 2026 13:25:17 +0000 Subject: [PATCH 2/2] fix(sales): TrackShip delivery uses skipPermissionCheck, keeping the state guard (review hardening) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- app/actions/sales.ts | 10 ++++-- docs/workflows.md | 8 +++-- lib/trackship.ts | 44 +++++++++++++++----------- tests/trackship-mark-delivered.test.ts | 28 ++++++++++------ 4 files changed, 58 insertions(+), 32 deletions(-) diff --git a/app/actions/sales.ts b/app/actions/sales.ts index 497b2b75..838cbae1 100644 --- a/app/actions/sales.ts +++ b/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({ diff --git a/docs/workflows.md b/docs/workflows.md index f5cb94c3..1d224982 100644 --- a/docs/workflows.md +++ b/docs/workflows.md @@ -34,9 +34,11 @@ rules. 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. 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 write, the guard rejects the change and the cron logs a +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 diff --git a/lib/trackship.ts b/lib/trackship.ts index fcd78609..08648f38 100644 --- a/lib/trackship.ts +++ b/lib/trackship.ts @@ -9,7 +9,6 @@ import { db } from '@/lib/db' import { logActivity } from '@/lib/activity-log' import { getOrderDeliveryStatus } from '@/lib/shopping' -import { INTERNAL_STATUS_TRANSITION_BYPASS } from '@/lib/sales/status-transition-bypass' // --------------------------------------------------------------------------- // TrackShip API (direct) @@ -81,8 +80,8 @@ type MarkDeliveredDeps = { transition: ( id: string, targetStatus: 'DELIVERED', - extra: undefined, - options: { internalBypassToken: symbol }, + extra?: { trackingNumber?: string; shipFromWarehouseId?: string }, + options?: { skipPermissionCheck?: boolean }, ) => Promise<{ success: boolean; error?: string }> log: typeof logActivity } @@ -91,21 +90,22 @@ type MarkDeliveredDeps = { * 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 under a lock — so an order - * that moved to CANCELLED / REFUNDED / etc. between the SHIPPED query and now - * is 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 the internal - * bypass token to skip the sales.process permission check while keeping the - * guard and side effects. Dependency-injected so the routing + skip-on-reject - * behaviour is unit-testable without the DB or external API. + * 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, { - internalBypassToken: INTERNAL_STATUS_TRANSITION_BYPASS, + skipPermissionCheck: true, }) if (result.success) { await deps.log({ @@ -170,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 @@ -200,12 +203,17 @@ export async function checkDeliveryStatus(): Promise<{ checked: number; delivere if (isDelivered) { const resolvedSource = useShoppingConnector ? 'shopping_connector' : 'trackship' - const { applySalesOrderStatusTransition } = await import('@/app/actions/sales') - const result = await markOrderDelivered( - { id: order.id, externalOrderNumber: order.externalOrderNumber, source: resolvedSource }, - { transition: applySalesOrderStatusTransition, log: logActivity }, - ) - if (result.delivered) delivered++ + 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) + } } } diff --git a/tests/trackship-mark-delivered.test.ts b/tests/trackship-mark-delivered.test.ts index fa585e9c..ef1ccd4b 100644 --- a/tests/trackship-mark-delivered.test.ts +++ b/tests/trackship-mark-delivered.test.ts @@ -2,21 +2,31 @@ import assert from 'node:assert/strict' import { test } from 'node:test' import { markOrderDelivered } from '@/lib/trackship' -const INTERNAL_STATUS_TRANSITION_BYPASS_DESC = 'internal-status-transition-bypass' +type TransitionCall = { + id: string + targetStatus: string + extra?: { trackingNumber?: string; shipFromWarehouseId?: string } + options?: { skipPermissionCheck?: boolean } +} function makeDeps(transitionResult: { success: boolean; error?: string }) { - const calls: { transition: unknown[]; logs: Array> } = { transition: [], logs: [] } + const calls: { transition: TransitionCall[]; logs: Array> } = { transition: [], logs: [] } const deps = { - transition: async (id: string, targetStatus: 'DELIVERED', extra: undefined, options: { internalBypassToken: symbol }) => { + 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) => { calls.logs.push(entry) }, } - return { deps: deps as never, calls } + return { deps, calls } } -test('routes delivery through the transition path with the bypass token (not a raw write)', async () => { +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' }, @@ -24,12 +34,12 @@ test('routes delivery through the transition path with the bypass token (not a r ) assert.equal(result.delivered, true) assert.equal(calls.transition.length, 1) - const call = calls.transition[0] as { id: string; targetStatus: string; options: { internalBypassToken: symbol } } + const call = calls.transition[0] assert.equal(call.id, 'so-1') assert.equal(call.targetStatus, 'DELIVERED') - // The bypass token is the symbol that lets the cron skip the permission check. - assert.equal(typeof call.options.internalBypassToken, 'symbol') - assert.equal(call.options.internalBypassToken.description, INTERNAL_STATUS_TRANSITION_BYPASS_DESC) + // 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')