fix(core/orders): stop the order upsert resetting fulfillmentState to NULL

[norbert-kulus-blockydevs]

Problem

order_records.fulfillmentState is a denormalized rollup over the order's shipments, pushed in from the shipping context by the narrow updateFulfillmentState UPDATE. Nothing on the ingestion path produces it: OrderRecordService.persistOrder builds new OrderRecord(...) with 11 positional arguments and fulfillmentState is the 12th, so it always defaulted to null.

OrderRecordRepository.upsert is a full-object TypeORM save() and toOrm mapped the column unconditionally, so every re-ingestion of an order - a poll re-read, a webhook-triggered sync, a manual re-sync - wrote that null over a 'dispatched' value already committed by the shipping context.

Operator-visible effect: a dispatched order reappeared as not-shipped. deriveSlaState treats a null rollup as not-shipped by design, so the order re-entered its ship-by SLA bucket, and the orders-list fulfillmentState filter matched it as not-shipped again.

Change

toOrm no longer maps fulfillmentState. Leaving the ORM entity property unset makes TypeORM omit the column from the generated statement, so the row's existing value survives and updateFulfillmentState is the column's sole writer. This is the shape cancelledAt already uses in the same method (#1984), three lines below the offending assignment.

The alternative was to read the existing record in persistOrder and carry the value onto the new instance. Rejected: upsert runs with no per-order lock and the shipping-side rollup write is unsynchronised with it, so a value committing between that read and the save would still be lost. Omitting the column is race-free and costs no extra read.

Documented consequence: the record returned by upsert reports fulfillmentState (like cancelledAt) as null whatever the row holds, because the column was never part of the statement. No caller reads it off the return value today; the port doc, the repository doc and the entity doc all state it and point at findById.

dispatchByAt is unaffected - it is re-derived from the source payload on every persist, so writing it is correct.

Tests

• order-record.repository.spec.ts: the property never reaches save(), including when the domain record carries 'dispatched' (guards a future caller reintroducing the clobber), plus the toDomain read-back.
• order-record.service.spec.ts: persistOrder never hands the upsert a non-null rollup.
• apps/api/test/integration/orders/order-record-fulfillment-state.int-spec.ts: persistOrder -> updateFulfillmentState('dispatched') -> persistOrder again still reads dispatched, and the re-polled order is excluded from the not-shipped and overdue filters while deriveSlaState reports none. Only a real save() proves TypeORM actually omits the column, which a mocked spec cannot.

docs/lessons.md gains the regression rule, since this is the second time the same class of defect landed on this table.

Known related finding (not fixed here)

syncStatus and syncAttempts have the same shape of problem and are reported separately rather than widened into this change:

• persistOrder / persistIncomingSnapshot pass [] for both and toOrm writes both unconditionally, so every re-ingestion truncates them. On the happy path syncStatus self-heals seconds later (OrderIngestionService calls updateSyncStatus per destination after syncOrder), but syncAttempts is genuine data loss on every re-poll - the per-destination failed -> retried -> synced timeline is replaced by a single fresh entry.
• If syncOrder throws or the process dies between the upsert and the writeback, syncStatus stays empty: the operator retry action 404s (OrderDestinationRetryService looks the destination row up in syncStatus) and FulfillmentStatusSyncService.findExternalOrderId can no longer resolve the destination order id, so fulfillment tracking for that order stops.
• The fix is not the trivial one-line exclusion used here: order_records.syncStatus is jsonb NOT NULL with no DB default, so omitting it from toOrm breaks the INSERT path. It needs either a migration adding a '[]' default (as syncAttempts already has) or an insert-vs-update split in the repository.

Verification

• pnpm --filter @openlinker/core type-check - clean
• pnpm --filter @openlinker/core lint - 0 errors (only pre-existing warnings in untouched files)
• pnpm --filter @openlinker/core test -- order-record - 89 passed; -- order-sla order-ingestion - 55 passed
• Integration tests were not run locally (Docker); CI covers them.

Closes #2101

[norbert-kulus-blockydevs]

Tech Lead review (PR #2107)

Reviewed against docs/code-review-guide.md, docs/engineering-standards.md, docs/architecture-overview.md § Cross-context dependencies in core, and the acceptance criteria in #2101. Read the branch code around the hunks; nothing was executed (no build/test run).

Findings

[BLOCKING] - apps/api/test/integration/orders/order-record-fulfillment-state.int-spec.ts:21,67,72,145-155

The new int-spec imports OrderRecordRepositoryPort from the bare @openlinker/core/orders barrel and resolves ORDER_RECORD_REPOSITORY_TOKEN out of the app injector. *RepositoryPort is an explicit deny shape for any importer under apps/{api,worker}/** (docs/architecture-overview.md § Cross-context dependencies in core: "Repository ports ... Intra-context contract ... cross-context callers go through I*Service"), and the walked scope explicitly includes apps/api/test/integration/**.
This is not just a doc violation, it breaks the gate: scripts/check-cross-context-imports.mjs classifies the name via DENY_PATTERNS = [/RepositoryPort$/, ...] and strips the inline type prefix before matching (parseImports), so the type-only import does not help. The exemption is an exact (repo-relative path, symbol) ALLOW_LIST key; the sibling entries are for apps/api/test/integration/order-dispatch-sla.int-spec.ts etc., and this new path (.../orders/order-record-fulfillment-state.int-spec.ts) has no entry. pnpm lint chains check:invariants -> check-cross-context-imports.mjs, so root lint and CI fail. This also fails #2101's own last acceptance criterion ("No architecture boundary violations").
The PR's local verification (pnpm --filter @openlinker/core lint) cannot catch this, since the offending file is in apps/api.
Fix without losing any coverage - drop the repository injection and use the service seam that exists for exactly this purpose. IOrderRecordService.findMany(filters, pagination) (order-record.service.interface.ts:98) is documented as "the cross-context surface ... repository ports are forbidden across context boundaries ..., so callers go through this service method instead", and getOrderRecord(internalOrderId) (:88) replaces findById. Both accept/return the same shapes the test already uses (OrderRecordFilters carries fulfillmentState and slaState; PaginatedOrderRecords.items is OrderRecord[]). Adding an ALLOW_LIST entry instead would be wrong: that list is scoped to pre-existing couplings tracked in #722, not new files.

[IMPORTANT] - acceptance criterion 4 of #2101 (syncStatus / syncAttempts)

The criterion is "checked for the same clobbering, and the finding is recorded (fixed here if trivial, filed separately if not)". The analysis is solid and correctly scoped out of this change, but it currently lives only in the PR body - I searched open+closed issues (syncAttempts upsert, syncStatus truncat, order_records/re-ingestion) and there is no filed follow-up. The PR body says the findings "are reported separately", which does not match the tracker state.
That matters because the finding as written is more severe than the one being fixed here: per-destination attempt history is destroyed on every re-poll, and on a crash between the upsert and the sync writeback syncStatus stays [], which 404s OrderDestinationRetryService and breaks FulfillmentStatusSyncService.findExternalOrderId. I confirmed the mechanism - toOrm (order-record.repository.ts:790-805) maps both arrays unconditionally and persistOrder passes [] for both (order-record.service.ts:126,138). File the issue (including the "why the one-line exclusion does not work here": syncStatus is jsonb NOT NULL with no default, unlike syncAttempts which has default: () => "'[]'") and link it from the PR body before merge.

[IMPORTANT] - libs/core/src/orders/application/interfaces/order-record.service.interface.ts (persistOrder, persistIncomingSnapshot JSDoc)

The "returned record reports fulfillmentState/cancelledAt as null whatever the row holds" caveat is documented in three places (repository port, repository method, entity) but not on the service interface, which is the only surface a cross-context caller is allowed to use (same architecture rule as the BLOCKING item). I verified the side effect is inert today: the sole upsert callers are persistOrder (order-record.service.ts:142) and persistIncomingSnapshot (:217), and both ingestion call sites (order-ingestion.service.ts:261,353) discard the returned record; on the cancelled path recordCancellationIfNeeded re-reads via findById, so that return is accurate there. The exposure is a future caller reading persistOrder(...).fulfillmentState and getting a silent null. One JSDoc line pointing at getOrderRecord closes it.

[SUGGESTION] - apps/api/test/integration/orders/order-record-fulfillment-state.int-spec.ts:156

expect(deriveSlaState(found!.dispatchByAt, found!.fulfillmentState, new Date())).toBe('none') also passes when dispatchByAt is null (see order-sla.ts: the dispatchByAt === null early return), and the slaState: 'overdue' exclusion at :148-149 has the same hole. So both assertions would stay green if a future change lost the ship-by deadline instead of the rollup. Add expect(found!.dispatchByAt).not.toBeNull() so the none verdict is provably attributable to the rollup. The not-shipped/dispatched filter assertions carry the real load, so this is hardening, not a defect.

[SUGGESTION] - libs/core/src/orders/infrastructure/persistence/repositories/__tests__/order-record.repository.spec.ts ("should read fulfillmentState back via toDomain when present on the ORM row")

The test is correct but sits inside the upsert describe and reads as if upsert's return carries a live rollup, which directly contradicts the consequence documented 15 lines above it in the repository (and in the port). Since save() returns the same entity instance that never had the property set, the real upsert return can never look like this. Retitle to name toDomain as the subject (or move it next to the other toDomain cases) so a future reader does not draw the opposite conclusion from the one the doc states.

Positive observations

• The cited precedent is real and load-bearing. cancelledAt is excluded from the same toOrm at order-record.repository.ts:820-829 ([IMPL] Backend — capture order cancellation as first-class record state #1984), directly below the removed fulfillmentState assignment, and apps/api/test/integration/orders/order-cancellation-record-state.int-spec.ts already proves empirically in CI that TypeORM omits an unset entity property from the generated UPDATE. So option B rests on a mechanism this repo already tests rather than on an assumption about TypeORM.
• The rejection of option A holds. upsert runs with no per-order lock (two ingestion paths legitimately race per architecture-overview.md § "Webhook = trigger, poll = reconciliation backstop") and updateFulfillmentState is an unsynchronised narrow UPDATE, so read-then-carry leaves a genuine lost-update window between the read and the save. Option B has no equivalent window.
• Blast radius is provably contained: toOrm has exactly one caller (upsert, :646) and this.repository.save( appears exactly once in the file, so the partial write set cannot leak into another write path. fulfillmentState is @Column({ type: 'varchar', nullable: true }) with no NOT NULL constraint, so the INSERT path degrades to NULL correctly and no migration is needed - and the PR is right that the same trick is unavailable for syncStatus.
• Nothing can now write NULL back: updateFulfillmentState takes a non-nullable FulfillmentRollupState and 'not-shipped' is a real member of the union, so the shipping context can still express a reset. No expressiveness lost.
• Good int-spec detail: sourceEventId differs across the two persists (evt-1 -> evt-2) and is asserted at the end, which proves a real UPDATE was generated instead of a no-op diff. The test cannot pass vacuously - that is exactly the failure mode this kind of test usually has.
• docs/lessons.md entry follows the documented Context / Problem / Rule / Applies to / Source format and is placed newest-first.

Summary

A correct, minimal, well-argued fix for a real operator-visible defect: toOrm stops mapping fulfillmentState, leaving updateFulfillmentState as the column's sole writer, exactly mirroring the cancelledAt (#1984) precedent in the same method. I verified the precedent, the race argument against option A, the containment of the partial write set (one toOrm caller, one save() site, nullable column so no migration), and that the documented side effect on upsert's return value is genuinely inert today (both ingestion call sites discard the record; the cancelled path re-reads). Acceptance criteria 1, 2, 3 and 5 are met. The single blocker is in the new integration test, not in the fix: it reaches for OrderRecordRepositoryPort from apps/api, which is a deny-shape cross-context import that fails scripts/check-cross-context-imports.mjs under root pnpm lint/CI and violates #2101's own "no architecture boundary violations" criterion - swapping to IOrderRecordService.findMany / getOrderRecord fixes it with no loss of coverage. Secondarily, the syncStatus/syncAttempts finding required by criterion 4 has not actually been filed anywhere durable.

Merge Readiness

Request changes

Priority fixes

1. Replace the OrderRecordRepositoryPort / ORDER_RECORD_REPOSITORY_TOKEN usage in the new int-spec with IOrderRecordService.findMany + getOrderRecord (BLOCKING - root pnpm lint and CI fail on the cross-context invariant).
2. File the syncStatus / syncAttempts re-ingestion-clobber issue and link it from the PR body, satisfying [BUG] CORE — an order re-poll resets fulfillmentState to NULL via the full-row upsert #2101 acceptance criterion 4.
3. Add the "return value is not authoritative for fulfillmentState / cancelledAt, re-read via getOrderRecord" note to IOrderRecordService.persistOrder / persistIncomingSnapshot.
4. Optional: assert dispatchByAt is non-null in the SLA int-spec case, and retitle the toDomain unit test so it does not read as a claim about upsert's return.

[norbert-kulus-blockydevs]

Addressed the blocking finding in apps/api/test/integration/orders/order-record-fulfillment-state.int-spec.ts.

What changed. The spec no longer imports OrderRecordRepositoryPort or resolves ORDER_RECORD_REPOSITORY_TOKEN. Both the filter reads and the single-record read now go through the published cross-context surface, IOrderRecordService:

• repository.findMany(filters, PAGE) -> orderRecordService.findMany(filters, PAGE) (all three calls: fulfillmentState: 'not-shipped', slaState: 'overdue', fulfillmentState: 'dispatched')
• repository.findById(order.id) -> orderRecordService.getOrderRecord(order.id)

That is the only change; the fix itself is untouched.

No coverage lost. OrderRecordService.findMany and getOrderRecord are pass-through delegations to OrderRecordRepositoryPort.findMany / findById, so the same queries run and return the same domain shapes. The spec still asserts the same three facts: the committed dispatched rollup survives a second persistOrder, the re-polled order does not re-enter the not-shipped list filter or the overdue SLA bucket, and the columns the re-pull does own are still refreshed (sourceEventId evt-1 -> evt-2).

Verification.

• node scripts/check-cross-context-imports.mjs passes clean: 1935 cross-context import(s) across 2470 file(s). All conform. Confirmed it was a real gate failure before the change: on the previous file the same script reported 1 violation(s) ... rule: matches deny pattern RepositoryPort$ at that exact line, so the fix closes the finding rather than moving it. No ALLOW_LIST entry was added.
• The spec type-checks clean against the apps/api config (apps/api/tsconfig.type-check.json excludes test/, so it was checked with the same compiler options over that one file), and is clean under eslint and prettier --check.
• check-service-interfaces, check-jest-integration-mappers and check-workspace-dep-declarations also pass.
• pnpm --filter @openlinker/core type-check passes; order-record.repository.spec.ts + order-record.service.spec.ts pass (63 tests). The int-spec itself still needs Docker, so it was not executed locally.

Still outstanding. The acceptance-criterion-4 follow-up for the syncStatus / syncAttempts re-ingestion clobber has not been filed as its own issue yet, so the tracker state still does not match the PR body. It is pending the repo owner's decision on filing, and the analysis (including why the one-line toOrm exclusion does not transfer, since syncStatus is jsonb NOT NULL with no DB default while syncAttempts defaults to '[]') is ready to move into an issue as soon as that is confirmed.

[piotrswierzy]

Tech-lead review — ✅ Approve (merge once CI actually runs)

Correct fix, and it follows the established precedent in this exact file rather than inventing a mechanism.

Confirmed: toOrm mapped entity.fulfillmentState = orderRecord.fulfillmentState while OrderRecordService.persistOrder passes only 11 constructor args — the rollup is the 12th — so every re-ingestion wrote the in-memory null over a committed 'dispatched'. Removing the assignment makes TypeORM omit the column, which is the identical mechanism as the cancelledAt exclusion three lines below (#1984) and race-safe under the same webhook-vs-poll two-writer scenario. updateFulfillmentState remains an absolute-set single-statement UPDATE, so there's no read-modify-write to lose.

Rejecting the read-before-write alternative in the code comment is the right call, and the consequence of the exclusion — upsert()'s returned record reporting the column as null regardless of the row — is documented on both the port and the impl, matching the cancelledAt treatment.

IMPORTANT — same defect class survives two columns over

order-record.repository.ts:790-796 — syncStatus / syncAttempts have the same shape, and this PR's own new lessons.md rule names it. Both are mapped in toOrm; persistOrder always passes [] for both (order-record.service.ts:125, :135); and updateSyncStatus is exactly the narrow out-of-band single-statement writer the rule describes. So a reconciliation re-poll of an already-dispatched order wipes its per-destination sync status and attempt history.

I haven't traced whether a caller repopulates it before an operator sees the list, so severity is unconfirmed — but the shape is. Worth a follow-up issue referencing #2101 rather than blocking here.

By contrast recordStatus / mappingFailureReason (also written narrowly, by updateItemResolutionFailure) are legitimately re-derived by ingestion — 'ready' on successful resolution — so they're not instances of this bug. Worth one sentence in lessons.md, or someone will eventually "fix" them and reintroduce a real regression.

Worth calling out

• The test pair is exactly right. The mocked-repo spec (expect(callArg.fulfillmentState).toBeUndefined()) proves intent, but only the new int-spec proves TypeORM actually omits the column from the generated statement — a mock can't establish that. And the second int-spec case asserts the operator-visible consequence (not-shipped filter + overdue SLA bucket + deriveSlaState agreeing with the SQL), which is what actually regressed rather than the mechanism.
• The "even when the domain record carries one" spec guards a future caller reintroducing the clobber.
• No schema change, so no migration and no backfill question: null ≡ not-shipped, and wrongly-nulled rows self-heal on the next updateFulfillmentState push.

CI: no check runs exist for 00a31fb at all (total_count: 0) — the workflow never fired for this head. Must be triggered and green before merge.

Merge readiness: ✅ Approve on the code; blocked only on CI actually running. Please file the syncStatus/syncAttempts follow-up.