feat(oms): Wave 0 — preconditions and correctness fixes (#2282–#2289)

[piotrswierzy]

Wave 0 of the OMS programme (backlog overview: docs/plans/oms-backlog-overview.md; execution ledger on the branch at docs/plans/oms-progress-ledger.md). Eight issues, each through the full per-issue process (plan → /pre-implement live-tree audit → plan review with all findings applied → Opus implementation → quality gate → diff review → merge), plus a wave-boundary consistency sweep over the seams between them, whose findings are applied in the closing commit.

What ships

• [TECH-DEBT] CORE/Orders — make persistOrder source-attribution-immutable (ADR-057 precondition) #2282 — persistOrder source attribution is immutable at the write path: upsert() swaps full-object save() for parameterized INSERT … ON CONFLICT — sourceConnectionId insert-only, sourceEventId same-source may advance, cross-source frozen (CASE), return contract preserved via fromRawRow resets. ADR-057's stated precondition.
• [BUG] CORE/Orders — a line that changes at the source between polls is silently lost (ingestion line-diff + internal amended fact) #2283 — re-ingestion line-diff records the internal amended fact: pure diffOrderAmendment keyed on items[].id+quantity, run pre-persist off the already-loaded record (survives item-resolution failure); lastAmendedAt/lastAmendmentChanges columns (single-writer, IS DISTINCT FROM no-op guard comparing changes only); PII-free by construction with one shared redactAddress rule; dated timeline entry. No amended union member yet — that lands with its consumer wave, enforced by [TECH-DEBT] CORE/Orders — never-default exhaustiveness on every OrderLifecycleEvent consumer before the union grows #2286's compile break.
• [BUG] CORE/Orders — a source-cancelled order can still be provisioned into a destination shop (WHERE cancelledAt IS NULL predicate) #2284 — source-cancelled orders are never provisioned: re-read guard before the destination fan-out; new terminal skipped_cancelled status (widening the existing OrderSyncStatusFilterValues, not a new const), structurally un-retryable; FE renders it neutral everywhere incl. the un-compiler-guarded order-health rollup.
• [BUG] CORE/Inventory — quantity-derived inv:{hash} idempotency key dedups corrective stock writes (latent oversell) #2285 — offer-quantity idempotency keys carry an observation token (observedAt, never wall-clock): closes the pause→restock→re-pause dedup swallow at Allegro's command-id layer; stale-offer-pause stamps staleAt; disjoint keyspaces across the deploy boundary.
• [TECH-DEBT] CORE/Orders — never-default exhaustiveness on every OrderLifecycleEvent consumer before the union grows #2286 — never-default exhaustiveness on all five OrderLifecycleEvent consumers + shared assertNever (@openlinker/shared/types); the relay input union is now derived from OrderLifecycleEvent (the sixth, undocumented restatement that would have defeated the guard). Adapters keep ADR-055 forward-compat (unsupported return + used never binding).
• [FEATURE] CORE/Orders — packedAt / packedByUserId: mark any order packed, whoever ships it (backend) #2287 — packedAt/packedByUserId (migration 1840000000000): first-write-wins conditional-UPDATE writers, POST/DELETE /orders/:id/packed (@Roles('admin','operator'), 200 on idempotent replay), shared toDto projection.
• [FEATURE] Frontend/Orders — packed toggle + column on the orders list and detail #2288 — the FE half: packed tick in the Shipment stack (first, chronological, tick-not-badge per [PRODUCT-DESIGN] Orders list — where packed and returns sit in the row (design delivered; needs a decision) #2081), always-rendered detail control (useWriteAccess('orders:write', …), deliberately not in the capability-gated shipment panel), dated timeline entry.
• [TASK] Integration/Allegro — spike: customer-returns feed shape (cursor vs date watermark) with an explicit kill condition #2289 — Allegro customer-returns spike: verdict 4A with the two-pass qualification (creation-cursor discovery + bounded re-read of non-terminal returns, since CustomerReturn has no updatedAt). Findings at docs/plans/analysis/SPIKE-2289-allegro-returns-feed.md; verdict comment on the issue.
• [TASK] Docs — reconcile the OMS design (DESIGN-oms-authority-model + ADRs 052-062) with the merged #2161 sales-documents rule engine #2298 — design freshness pass (R2): six wrong-about-shipped-code statements fixed across DESIGN + ADRs 052/054/056; adopted amendment to ADR-054 — routing rules are rows in the plugin's own oms_routing_rules table, superseding Connection.config.routing jsonb; design artifact re-synced.

Numbers

83+ files, ~4.9k insertions / ~290 deletions before the boundary commit; 2 migrations (1840000000000, 1841000000000 — synthetic-sequential, verified by check-migration-timestamps); every gate green per issue (core 247/3057, api 106/1392, web 355/3651+1s at final state) plus per-issue int-specs (cancelled-skip e2e, packed 5-case, amendment 4-case, attribution immutability).

Wave gates this unlocks

Wave 1a's entry criteria (#2284/#2286/#2283 merged; #2298 resolved) are all satisfied by this PR. Per the stacked-wave model, oms-programme-wave-1 continues from this branch's tip; merging this PR then folds main forward.

Closes #2282
Closes #2283
Closes #2284
Closes #2285
Closes #2286
Closes #2287
Closes #2288
Closes #2289
Closes #2298

🤖 Generated with Claude Code

[piotrswierzy]

Tech-lead review — 🔄 Approve with changes

(Reviewed as tech lead; GitHub does not accept an approving review from the PR author's own account, so this is submitted as a comment. Treat the verdict as "approve once the BLOCKING item below is resolved".)

Scope: 83 files, +4965/−285, 9 issues, 2 migrations. Reviewed the write path (#2282), amendment diff + redaction (#2283), cancellation guard (#2284), idempotency token (#2285), exhaustiveness (#2286), packed slice (#2287/#2288), and the FE rollup.

---

🔴 BLOCKING — migration timestamp collides with open PR #2439

apps/api/src/migrations/1840000000000-add-order-record-packed.ts

PR #2439 (fix(prestashop): read the order's real currency…, also based on main, also open) introduces apps/api/src/migrations/1840000000000-reset-fx-stamp-for-mislabelled-prestashop-orders.ts. Both branches picked 1840000000000 as the next free synthetic prefix after main's tail (1839000000000) — correctly, in isolation.

Whichever merges second fails scripts/check-migration-timestamps.mjs on rule 1 (uniqueness across the union) and rule 3 (strictly greater than every migration on origin/main). Both rules are chained into pnpm lint, so this surfaces as a red pre-commit/CI on the loser's next push rather than as a silent problem — which is the good outcome, but it still blocks a merge.

Worth stating explicitly because migrations.md warns about exactly this window: "rare, but possible in branch-merge windows". It is not rare when two long-lived branches both fork from the same tail.

Resolution — needs a decision on merge order, not just a renumber:

• If #2439 lands first: this branch renumbers to 1842000000000 / 1843000000000 (its own pair currently occupies 1840/1841), updating both filenames and both class suffixes.
• If this lands first: #2439 renumbers to 1842000000000.

Since #2439 is a single small fix with a live-verified backfill and this is a nine-issue wave that Wave 1a already stacks on, landing #2439 first and renumbering here is the cheaper sequencing. Either way, the renumber is mechanical (filename prefix + class suffix), and neither migration has run anywhere yet, so no migrations-table reconciliation is needed.

---

What's right, and non-obviously so

• #2282 is the right shape, and the reasoning for raw SQL is correct. sourceConnectionId is uuid NOT NULL with no DB default, so it must be on the INSERT half and absent from DO UPDATE — a shape save() genuinely cannot express, and read-before-write would reopen the webhook/poll race the toOrm doctrine exists to close. The sourceEventId CASE (same-source advances, cross-source frozen) preserves pre-#2282 behaviour exactly where it was correct and freezes it only where it was wrong. Keeping the caller-side ADR-017 echo guard as defence in depth rather than deleting it is right.
• fromRawRow resetting the out-of-band columns is the subtle call, and it went the right way. RETURNING * genuinely carries the true values, and passing them through would have been a silent behaviour change to both OrderRecordService call sites — the documented contract ("those columns read empty; re-read via findById") is what callers were built against. Verified there are no column transformers on the write-set columns, so Object.assign off the raw driver row is safe (all text/uuid/varchar/jsonb/timestamptz; the decimal column is reset anyway).
• #2283's single redactAddress rule is the load-bearing detail, and the header states the failure mode precisely: two implementations means every poll compares raw against redacted and reports a phantom shipping-address change forever. Running the diff pre-persist off the already-loaded record (so it survives item-resolution failure), and excluding both columns from toOrm so the detecting re-poll cannot erase its own finding, are both correct.
• #2284 re-reads rather than threading the ingestion snapshot — right, because that snapshot is taken before item resolution and persist, i.e. stale in exactly the race being closed. Propagating a read failure instead of proceeding is the correct asymmetry.
• #2285's observedAt MUST NOT be wall-clock is documented on the field itself, where the next implementer will read it, not just in the PR body. That is the one rule that, if broken, silently converts dedup into a write-per-tick.
• #2286 deriving the relay input union from OrderLifecycleEvent rather than adding a sixth restatement is the fix that actually makes the guard hold; adding assertNever to five sites while leaving an independent copy of the union would have been guard theatre.
• FE rollup: counting skipped_cancelled in its own branch — with the comment explaining that the else catch-all is not compiler-guarded — is exactly the right treatment, and refusing to add a sixth OrderHealth bucket keeps the #2081 partition invariant (KPI cards must sum to total) intact.

---

🟡 IMPORTANT

1. assertNever's runtime JSON.stringify can render untrusted payload content into an exception message. libs/shared/src/types/exhaustive.types.ts — the value reaching a default: arm on the relay path is an OrderLifecycleEvent, which is source-derived. If any consumer's error propagates into a persisted sync_jobs failure message or an operator-facing surface, that renders the whole event object, which on some paths carries buyer address fields. Given #2283 in this same PR is explicitly PII-free by construction, the asymmetry is worth closing: render only the discriminant ((value as {type?: unknown})?.type) rather than the full object. The compile-time guarantee — the actual point — is unaffected.

2. recordAmendment returns void, so the IS DISTINCT FROM no-op is unobservable. order-record.repository.ts — the guard is described as the second line of defence against a source re-reporting the same amendment every poll. Returning (result.affected ?? 0) > 0 (as markPacked / clearPacked do) would let the caller log the suppression, which is the only way anyone would ever learn the primary changes.length > 0 gate had stopped working. Every sibling guarded writer in this file already returns a boolean.

🟢 SUGGESTION

3. inventory-sync.service.ts — the inventory_quantity_key_unversioned warn fires per item, so a bulk propagation over a connection whose callers all omit observedAt emits one line per offer. A per-command aggregate (count + connection) would keep the degradation observable without a log flood on exactly the large batch where it is most likely.

---

Checks

• Layering: repository writes in infrastructure, policy in application services, controller thin. OrderSyncService → IOrderRecordService is a same-context service-interface dependency. ✅
• assertNever in libs/shared/src/types admitted under the engineering-standards.md pure-rule exception, and the docblock says so rather than setting a silent precedent. ✅
• Migrations: both have up() + down(); class suffix matches filename prefix; synthetic-sequential. ✅ (prefix uniqueness — see BLOCKING)
• Authz: @Roles('admin','operator') on both packed routes; FE gates on useWriteAccess('orders:write', …) and correctly keeps the control out of the capability-gated shipment panel (packing is not a carrier capability). ✅
• SQL: every statement parameterized; no interpolation. ✅
• Tests: 4 int-specs (attribution immutability, packed, amendment, cancelled-skip e2e) plus unit coverage on each pure function. ✅

Once the timestamp collision is sequenced, this is ready.

[piotrswierzy]

Migration-collision update — it resolved against you, and it also moved

Since my review, main merged #2439, so the collision I flagged as a merge-order question is now settled fact:

• main owns 1840000000000 (1840000000000-reset-fx-stamp-for-mislabelled-prestashop-orders.ts). This branch's 1840000000000-add-order-record-packed.ts now fails rule 1 (uniqueness) and rule 3 (strictly greater than main's newest).
• 1841000000000 is no longer free either. feat(core,integrations,web,worker): per-line tax rate from the master to the invoice and the receipt #2260 rebased onto the new main and took 1841000000000–1841000000004, so 1841000000000-add-order-record-amendment.ts would collide with 1841000000000-add-product-tax-rate.ts there.

So both files need moving, and the obvious "shift by one" lands straight into #2260's block.

1842000000000 / 1843000000000 clears main's tail and #2260's range in one step. Filename prefix and class-name suffix on each; nothing has run anywhere, so no migrations-table reconciliation.

Two knock-on notes:

• feat(oms): Wave 1a — vocabulary leaves + derived order lifecycle phase (epic #2312) #2441 (Wave 1a) inherits this. It stacks on this branch, so its pnpm lint cannot pass until the renumber lands here. Flagged on that PR too.
• feat(orders,analytics): order analytics read model + sales/channel/top-products endpoints + per-line tax rate + net tax basis #2014 has the same class of problem in the other direction — its 1837000000000-add-order-analytics-read-model.ts sorts below main's tail and has since before main moved (I missed it in my review there and have posted a correction). Worth knowing if you sequence these together, since it also wants a free slot above main.

Current claims on the range, for whoever renumbers first:

Prefix	Owner
1840000000000	main (merged, #2439)
1840000000001	#2260 (analytics read model, rebased)
1841000000000–1841000000004	#2260 (tax-rate block)
1842000000000+	free

My review's other two items (assertNever rendering a source-derived event into an exception message; recordAmendment returning void so its no-op guard is unobservable) are unchanged and still worth folding in.

[piotrswierzy]

Re-check after 53cbf6f7 — migration blocker cleared ✅, with one latent ordering dependency

The renumber landed correctly:

File	Class suffix	vs main tail (1840000000000)
1841000000000-add-order-record-amendment.ts	AddOrderRecordAmendment1841000000000 ✅	greater ✅
1842000000000-add-order-record-packed.ts	AddOrderRecordPacked1842000000000 ✅	greater ✅

Both filename prefixes and class suffixes agree, both clear main's newest, and the tree has no duplicate prefix — so rules 1, 2 and 3 all pass against main as it stands today. The blocking item from my review is resolved.

🟡 One thing to know before merging out of order

1841000000000 is free on main but is not free on #2014's branch, which absorbed #2260 and carries 1841000000000-add-product-tax-rate.ts through 1841000000006.

So the two branches are now order-dependent:

• This merges first → fine. feat(orders,analytics): order analytics read model + sales/channel/top-products endpoints + per-line tax rate + net tax basis #2014 then has to renumber its 1841… block above whatever tail this leaves (1843000000000+).
• feat(orders,analytics): order analytics read model + sales/channel/top-products endpoints + per-line tax rate + net tax basis #2014 merges first → main's tail becomes 1841000000006, and both of this branch's migrations fail rule 3 (neither 1841000000000 nor 1842000000000 is strictly greater), plus 1841000000000 collides outright on rule 1. This branch would need a second renumber.

Not something to change now — a renumber to pre-empt a merge order that hasn't been decided just moves the problem. Worth deciding the order deliberately, given #2441 stacks on this branch and #2014 is a five-PR merge that presumably wants to land soon. Merging this one first is the cheaper sequence: two files here versus seven there.

Still open from my original review

Neither has been addressed and both stand:

• assertNever renders a source-derived event into its exception message via JSON.stringify(value). On the relay path that value is an OrderLifecycleEvent carrying buyer address fields, and the message can reach a persisted sync_jobs failure or an operator surface. Render the discriminant only. This gets more load-bearing with feat(oms): Wave 1a — vocabulary leaves + derived order lifecycle phase (epic #2312) #2441, which adds several new unions that will grow assertNever call sites.
• recordAmendment returns void, so its IS DISTINCT FROM no-op guard is unobservable — the caller can't log that the primary changes.length > 0 gate stopped suppressing. Every sibling guarded writer in that repository returns a boolean.

Verdict unchanged: approve once those two are folded in (or explicitly deferred with a reason) — the migration objection is gone.