refactor(core/inventory): column-scope the inventory upsert's existing-row write

[piotrswierzy]

What

InventoryRepository.upsert wrote an existing inventory_items row through save() after toOrmEntity populated every column, so which columns the master sync may touch was an emergent property of TypeORM's diffing — nothing declared it, nothing failed when it changed. This is the row every published quantity derives from, so the failure mode is an oversell or a mass unpublish, produced with zero compile errors.

Three column groups are now declared and the update writes exactly one of them:

Group	Columns	Why
Identity	id, productId, productVariantId, locationId	These are the lookup key — writing them back is a no-op at best, a row-identity change at worst
Master-owned	availableQuantity, reservedQuantity, isStale	What the master sync is entitled to write
DB-managed	updatedAt	Excluded from both the SET clause and the insert mapping, so @UpdateDateColumn keeps stamping it

updatedAt was actually being written wrong — this is a fix, not only hardening

The issue framed this as pure hardening, and so did my first draft. Both were wrong.

toOrmEntity assigned item.updatedAt, and on the save() path that value reached the database: SubjectChangedColumnsComputer has column.isUpdateDate explicitly commented out of its skip list, so an assigned update-date enters the change map and suppresses CURRENT_TIMESTAMP. (The SubjectExecutor overwrite that appears to prevent this sits in the MongoDB branch and never runs on Postgres — a trap, because it reads like the general case. I cited it wrongly at first.)

So an @UpdateDateColumn has been carrying a master-supplied value. It has never bitten only because both shipped inventory-master adapters leave the field undefined and MasterInventorySyncService fills it with ?? new Date(). An adapter that starts reporting updatedAt would silently take over the column — and InventorySyncService derives the propagation job's dedupe key from it, so a master reporting a stable timestamp while quantity moved would collide the key and drop the propagation.

Verified empirically rather than by reading: reinstating updatedAt in the SET clause makes the integration test persist a 2020 master timestamp.

Excluding it means the returned entity can no longer reconstruct the value, hence the query-builder form with .returning(['updatedAt']) rather than the repository.update(id, …) idiom used elsewhere — this file already uses both the builder and .returning() for markStaleExceptVariants.

Both branches changed, not just the update branch

The same exclusion applies to the insert path: toOrmEntity no longer assigns updatedAt at all, because assigning an @UpdateDateColumn on an INSERT persists the master's timestamp exactly as it used to on UPDATE.

That makes the stamp DB-only on both paths, so both must verify they got one. The update branch throws when the driver ignores RETURNING; the insert branch now throws when save() returns no usable stamp, rather than handing back an InventoryItem whose updatedAt is typed Date but is undefined — into the same dedupe-key consumer. Both resolve through one helper, which additionally rejects an unparseable value: the raw key is literally updatedAt only because no namingStrategy is configured, and under a snake_case strategy the row object stays truthy while the property reads undefined, so guarding the row was never sufficient.

Failing loudly instead of returning a phantom row

A scoped UPDATE cannot resurrect a deleted row the way save() could (it fell back to an INSERT). Zero affected rows now raises InventoryRowVanishedError instead of returning an InventoryItem for a row that no longer exists — which InventorySyncService would turn into a marketplace propagation for absent stock. Unreachable today (the port has no delete, the staleness sweep is a soft update, both FKs are ON DELETE NO ACTION), so it guards a future delete path.

isStale stays owned — and why that is safe is now written down

isStale is written false on every upsert (the sole domain construction outside this file omits the argument, so the constructor default applies). The existing comment cites #1478 "clears the flag when a variant reappears", but that states intent. The actual guarantee is ordering: MasterInventorySyncService runs its setInventory loop before pruneStaleVariants, and the prune stales exactly the variants the loop did not report — so the sets are disjoint and an upsert cannot un-stale a row the same run just flagged. That precondition is now recorded next to the set it constrains, and restated on the port where a new caller will actually read it.

Two mechanisms for one defect class, now recorded

This is the fourth fix in the "a narrow out-of-band writer gets clobbered by a broad upsert" family (#1984 cancelledAt, #2101 fulfillmentState, #2140 syncStatus/syncAttempts), but the first to use allowlisting rather than omission.

docs/lessons.md described only omission. It now names both, says when each earns its weight — allowlisting buys a build failure on a newly-added column, which omission structurally cannot, since omission is the absence of a line and nothing fails when the next author adds one — and lists this repository under Applies to. Without that, the next author picks between the two mechanisms by coin flip.

Tests

The AC asks for a test that "a column outside the owned set is not modified". No such column exists — every column is identity, owned, or DB-managed — so that test would have to invent one and prove nothing. Instead:

• Behavioural: the SET payload has exactly the owned keys (both directions), each value sourced from the item, keyed on the matched row's id; no identity or DB-managed column appears; save is not called.
• Contract: every column declared on the ORM entity is classified into exactly one group — so adding a column fails the build until someone decides where it belongs. Same "must be updated deliberately" shape as route-lazy.test.ts's expected-count assertion.
• Failure paths: zero-affected, ignored-RETURNING, an unparseable returned stamp, and both insert sub-branches (direct and regenerated-id) each assert their throw.
• Integration: inventory-stale-prune.int-spec.ts passes a deliberately stale master timestamp and asserts the persisted updatedAt is strictly greater than both it and the pre-update value — the only assertion shape that proves TypeORM really omitted the column, which a mocked spec cannot.

All three guards are mutation-verified, not merely green: adding a column to the entity fails the contract spec by name, slipping updatedAt back into the SET clause fails the behavioural specs, and the same mutation fails the integration assertion.

Gate

Rebased onto current main (the previous red CI was a stale base — a PHP Unit Tests failure on a PR touching zero PHP). pnpm lint 0 errors, pnpm type-check clean, pnpm test exit 0, 5 inventory suites / 74 tests green.

Closes #2071

🤖 Generated with Claude Code

[piotrswierzy]

Tech Lead review — 🔄 Approve with changes

(Posted as a comment because GitHub rejects APPROVE on one's own PR. No blocking defects.)

Third instance of the "narrow out-of-band writer gets clobbered by a broad upsert" family, after #2107 and #2141, this time on inventory_items. Notably it uses a different mechanism from its two predecessors: instead of dropping the assignment in toOrm so TypeORM omits the column, it replaces the existing-row save() with an explicit column-scoped UPDATE … SET availableQuantity, reservedQuantity, isStale WHERE id = :id RETURNING updatedAt. I checked the mechanics against the files and found no correctness defect. CI 9/9.

Race-safety — confirmed. findByProductAndVariant is used only to resolve identity for the WHERE id; every written column is an absolute set from the inbound item, so this is not the read-modify-write shape the lessons rule forbids. Owners of the scoped-out columns:

• updatedAt → the DB (@UpdateDateColumn, inventory-item.orm-entity.ts:70). TypeORM's UpdateQueryBuilder appends the auto-stamp only when the column is absent from SET, so the exclusion is load-bearing — and InventorySyncService derives its propagation dedupe key from that value.
• id / productId / productVariantId / locationId → lookup key, never writable.
• isStale → out-of-band writer is markStaleExceptVariants, a single bulk absolute-set UPDATE. This PR deliberately keeps isStale in the written set, and the stated precondition holds: upsert's only caller is InventoryService.setInventory, whose only caller is master-inventory-sync.service.ts:101, which runs before the prune at :131/:219 — so the two write sets are disjoint. That is also squarely inside the lessons rule's carve-out (ingestion legitimately re-derives "the master reported this variant ⇒ live", the recordStatus shape), so I agree it isn't an instance of the bug.

#2141's three refinements. Whole method swept — yes, and the insert branch is explicitly reasoned about and left un-scoped, which is right (an INSERT has no competing writer). ?? [] guards — correctly N/A: no array/jsonb column on this entity, and the update branch never routes through toDomain. DB-default provenance — no NOT NULL column newly depends on a default, so the absence of a migration is a conclusion rather than an omission.

Int-spec — present and the right kind. inventory-stale-prune.int-spec.ts pushes a deliberately ancient master timestamp (2020-01-01) through a real save path and asserts the committed updatedAt is newer than both the prior row and the master value. That is the only assertion shape that proves the column was actually omitted; a mocked spec cannot.

IMPORTANT

1. libs/core/src/inventory/domain/ports/inventory-repository.port.ts:52 — the upsert doc comment is unchanged. Both predecessors documented the exclusion on the port and the impl. Here the port hides three caller-observable changes: the returned item's updatedAt is now the DB-stamped value rather than the inbound one, and upsert can now throw (InventoryRowVanishedError, plus a bare Error on a driver that ignores RETURNING) where it previously always resolved. Please state on the port that the master-owned write set is scoped, that updatedAt is DB-authoritative, and that the method throws.

2. docs/lessons.md:29-32 — not updated, and this is the gap I'd most want closed before merge. The existing rule prescribes the toOrm-omission mechanism and its Applies to names only order-record.repository.ts. This PR introduces a second, different mechanism for the same defect class on a second table. Whichever is now preferred, the ledger should say so and list inventory.repository.ts — otherwise instance #4 picks between save()-omission and a scoped UPDATE arbitrarily, and we re-litigate the mechanism instead of applying it.

SUGGESTION

3. The missing-RETURNING branch throws a bare Error('inventory_upsert_missing_returning …') from a repository, where standards call for a domain exception — and the sibling path in the same block correctly uses InventoryRowVanishedError. Either a second named error or a discriminating reason on the existing one.

4. raw[0].updatedAt is key-name-coupled. It works today only because the project sets no namingStrategy, so the column is literally "updatedAt". Adopt snake_case later and returnedRow stays truthy while returnedRow.updatedAt becomes undefined → new Date(undefined) → a silent Invalid Date past both guards. A Number.isNaN(persistedUpdatedAt.getTime()) check makes that fail loudly.

5. The INVENTORY_*_COLUMNS groups are exported purely for the spec. Acceptable, but they're infrastructure detail — a one-line "exported for the classification spec, not for callers" note keeps them from drifting into the public surface. (Correctly absent from the barrel; only the error is exported there.)

Positive observations

• The classification spec is the strongest artefact here. Requiring every declared entity column to fall into exactly one group means adding a column now fails the build until someone classifies it — a materially better guarantee than either predecessor got, and the thing that makes this mechanism worth its extra weight.
• The comment forbidding "DRYing" the literal set({...}) into an object built from the constant — because the literal is what gives TypeORM's key type-checking something to check — is exactly right, and pre-empts a plausible future regression.
• Both failure branches are justified by what the caller would do with a wrong value (enqueue propagation for absent stock / poison the dedupe key), not by defensive habit. InventoryRowVanishedError's header states why it should be unreachable and cites the evidence.

CI: all 9 checks green on 339a680.

Merge readiness: 🔄 ready once the port doc and the docs/lessons.md update land. Both small; the second is what stops instance #4 from starting over.

[piotrswierzy]

Delta re-review (339a6808 → 522e4269) — ❌ Not ready

(Posted as a comment because GitHub rejects REQUEST_CHANGES on one's own PR. Treat this as blocking: two prior findings are untouched, there's a new asymmetry, and CI is red.)

One new commit: fix(core/inventory): stop the insert path persisting the master's updatedAt.

Prior findings

Finding	Status	Evidence
IMPORTANT 1 — inventory-repository.port.ts:52 upsert doc	❌ Not resolved	The port file isn't in the diff at all. The doc still reads @returns Upserted inventory item domain entity, with no mention of the column-scoped write set, the DB-authoritative updatedAt, or the two new throws
IMPORTANT 2 — docs/lessons.md	❌ Not resolved	Untouched. Applies to still names only order-record.repository.ts, and the ledger still describes one mechanism while this PR ships a second
SUGGESTION a — bare Error	✅ Resolved	New inventory-returning-unsupported.error.ts, thrown at :318, exported from the barrel, spec asserts the type rather than a message regex
SUGGESTION b — raw[0].updatedAt key coupling	⬜ Not resolved	:320-323 still lacks a Number.isNaN(...getTime()) guard
SUGGESTION c — INVENTORY_*_COLUMNS export note	⬜ Not resolved	Mitigated by their absence from the barrel, so reach is intra-context

Invariant re-established. MasterInventorySyncService and InventoryService are untouched, so the setInventory-before-prune ordering that makes isStale safe in the owned set still holds, and the precondition is still recorded at :61-68. The classification spec still forces classification — inventory.repository.spec.ts:289-302 reads getMetadataArgsStorage().columns and toEquals the union of the three groups, so a new entity column fails by name. Both good.

NEW — IMPORTANT

1. inventory.repository.ts:397-402 — the insert path now has no counterpart to the update path's RETURNING guard. Dropping entity.updatedAt = item.updatedAt from toOrmEntity is correct and proven on Postgres by the new int-spec. But toDomain(saved) at :389/:394 now reads entity.updatedAt straight back, and on any driver where TypeORM doesn't return the inserted row that property is undefined while InventoryItem.updatedAt is typed Date. The update path throws loudly for exactly this condition (InventoryReturningUnsupportedError); the insert path hands a malformed entity to the same propagation-dedupe-key consumer.

That is precisely trap (1) in the existing docs/lessons.md rule — "an excluded column needs a guard where toDomain reads it" — which is a second, independent reason finding 2 above needs closing rather than a coincidence. Either guard symmetrically or state why the insert path is exempt.

2. The PR body is now stale. This commit changes the insert path, not just the update path, but the summary table still describes updatedAt as excluded only from "the SET clause". Worth fixing so a reader of the merge commit sees both.

SUGGESTION

inventory-row-vanished.error.ts:24-28 dropped the cause?: unknown parameter. Correct — no caller passes one — and safe, since the class was introduced by this same PR so nothing external depends on the old shape. Noting it only so the signature change is on the record.

CI — red on 522e4269

Lint, Type Check, Build, Test, PHP Unit Tests all failing; Integration Tests, Docker Smoke, Scaffolded Adapter Builds pass. Plausibly a stale base rather than a branch defect: mergeable_state: "behind", base bc59f43e, and a PHP failure on a PR touching zero PHP is otherwise inexplicable. Sibling #2150, based one commit later, is fully green. Rebase onto c9231c9b and re-run. If Lint stays red, the if (Error.captureStackTrace) guard added in both new exception files is the candidate — no-unnecessary-condition, since the property is non-optional in the TS lib types.

One cross-PR note

#2150 currently deletes the docs/lessons.md rule you'd be extending here (verified: present on main, absent on its branch). I've asked that PR to restore it. Worth coordinating the merge order — if #2150 lands first unfixed, finding 2 has nothing to attach to.

Merge readiness: ❌ Not ready. Both untouched findings are single-file doc edits, and the second is arguably the more valuable half of this PR: the point of introducing a second mechanism for a known defect class is that the next author can tell which one to reach for. Add the insert-path guard or its exemption comment, rebase, confirm green.