fix(management): bind numeric inventory writes at the service, not the transport #78
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/71-69-service-layer-bounds"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #71. Closes #69.
Merging this deploys the management container to production via
build-publish.yml. No CI runs on pull requests here — that is #73, in flight this wave — so every result below is a local command, quoted as such. Nothing has checked this branch automatically.The structural mistake both issues point at
Every numeric bound added by PR #68 was attached to a transport — a REST schema, an MCP tool registration. None was attached to the service that writes the column. That is why the write-path count in #68 was wrong four times running: four, then six, then ten, then a tenth that still bypassed the bound.
Enumerated from the service layer rather than from the diff, each of the three services that write these columns had exactly three callers, and only the MCP one was bounded:
adjustStock(stock)create_stock_adjustmentcreateStockAdjustmentexecute-stock-adjust.tscreatePurchase(stock,buy_price)createPurchaseWithInventory/updatePurchaseexecute-purchase.tscreateOrder(stockvia FIFO,priceSold)createOrderWithItemsexecute-order.tsBounds now live at the chokepoint every caller routes through, reusing
MAX_RUPIAH. The transport schemas stay — they produce the readable field-named 4xx PR #68 established, and are now defence in depth rather than the only defence.Why the bot path is the one that mattered
POST /api/whatsapp/webhookis live and api-key gated. Itsconfirmnode validates no numbers, and the numbers themselves were model output:classify-intent.tsdeclaredqty/buyPrice/priceSold/deltaas barez.number(), andvision-llm.tstyped a scanned receipt line asquantity: z.number(), unitPrice: z.number(), mapping them straight ontocreatePurchase→inventory.buy_price.So the untrusted input was not a hostile user. It was a vision model reading a phone photo of a receipt, and a mis-read total — the ordinary failure mode of OCR — landed on the cost column every COGS and profit figure derives from. Both schemas now declare integer bounds, which also reach the model through
withStructuredOutput.Product decision recorded: a refused number rejects the whole record and names the line, rather than dropping the line and reporting success. A purchase that looks saved but is missing a row biases every margin computed from it, with nothing to notice. The server actions restate a service rejection so it survives Next's redaction of uncaught server-action errors.
#69 — the three defects
createStockAdjustmenthad no zod at all — the only inventory write path with none, protected solely by the browser's implicitstep="1", which is accidental and evaporates understep="0.01",noValidate, or any non-browser caller.1.500inJumlahmade the modal's own preview offerPerubahan: 12 → 13.5 (+1.5)with save enabled; clicking save hitstepMismatchand did nothing — no server call, no mutation, no error. The operator was shown a specific outcome, allowed to commit to it, and got silence.Jumlahnow goes throughparseInt4Inputlike every other numeric field, the preview only renders an outcome the save can reach, and a save the service refuses shows the reason.delta: -99on stock 12 committed-87. A negative delta is legitimate, so a schema cannot express this — it sees the requested change, not the current row. The decision now lives inStockChangeSchema, which takes the current stock as an argument, which is also what makes it unit-testable without a database.The public consequence, and the second branch is the harmful one:
allow_indentavailabilityfalsehabistrueindentapps/store/app/products/[slug]/add-to-cart.tsxdisables the cart forhabisonly;indentfalls through to the full cart flow.Verified
The lint result is not clean and is reported as such. All three warnings are pre-existing on
origin/main— verified by running the same binary 2.4.16 againstorigin/main's copy of each file, which reports the identical rule at the identical selector, line-shifted only by lines the diff added above:useOptionalChaininexecute-order.tsandexecute-stock-adjust.ts, andnoUnusedImportsonrecordMovementinorders/service.ts. Zero introduced. Left alone as formatter drift on lines the diff does not touch.MAX_RUPIAHremains single-source — one literal repo-wide.Deviations
noUnusedImportsonorders/service.tsis pre-existing but sits in a file this diff edits. Judged as drift rather than cleanup; flag it if you disagree, since the argument cuts both ways when the file is already open.StockChangeSchemataking current stock as an argument means "delta −99 against stock 12" is an assertion, not a fixture. The end-to-end path still wants exercising against real Postgres.lib/mcp/tools/purchases.ts:159echoes a raw drizzleFailed query:to the MCP client, which is schema disclosure to an LLM agent. #70 — the Hono app has noonError(PR #76, in flight; contract agreed overhub: services throw a plain zodZodErrorand that handler maps it to #68's incumbent envelope).VERDICT: request-changes
Round 1. Nobody has reviewed this code before, so I re-derived every claim rather than spot-checking. The thesis is right and most of the execution is excellent — but one central claim is false in production, and the change has no executable defence of the thing it exists to do.
Re-ran the matrix independently:
pnpm --filter garage test→ 10 files, 280 passed;pnpm exec tsc --noEmitfromapps/management→ exit 0 (unpiped);biome check(bare nix 2.4.16, real files,PIPESTATUS[0]) on the 16 changed files →Checked 16 files. Found 3 warnings., exit 0. All three warnings reproduce on anorigin/mainworktree at identical selectors, shifted +1 line each (execute-order.ts10→11,execute-stock-adjust.ts9→10,orders/service.ts9→10). Zero introduced — confirmed.git grep '2_147_483_647\|2147483647' -- apps packages→ one line,lib/currency.ts:24.MAX_RUPIAHis single-source.Caller enumeration: complete for the services, incomplete for the column
Re-derived from the service layer, not the diff. The table is accurate:
adjustStock←mcp/tools/stock-movements.ts:43,stock-movements/actions.ts:31,bot/nodes/execute-stock-adjust.ts:20.createPurchase←mcp/tools/purchases.ts:63,purchases/actions.ts:27,bot/nodes/execute-purchase.ts:51.createOrder←mcp/tools/orders.ts:89,orders/actions.ts:24,bot/nodes/execute-order.ts:40. Three each, no fourth caller, and REST correctly folds into the action (app/api/[[...route]]/purchases.ts:53,orders.ts:69) rather than being a fourth door.But the acceptance criterion is about the column, and
purchases/service.tsholds threeinventory.stockwrite sites. The diff guards one. Seeblocker-adjacent finding on L126 below.What I verified as genuinely correct
Called all three services directly, bypassing every transport, inside savepoints inside a rolled-back transaction, with a full-table
md5(string_agg(...))+ counts acrossinventory,purchases,purchase_items,inventory_lots,stock_movements,orders,order_itemsafter every probe. 24/24 probes: a readableZodErrorwith the correct field path, and side-effectsNONEon all seven tables. TheFailed query:shape from #71 is gone, including[items.0.buyPrice]-style line naming,[items.2.qty]indices,not_a_numberoninventoryId: "abc", and both resulting-stock branches. A partially-applied purchase never happened.withStructuredOutputclaim — true, and it is the real behavioural claim it says it is. Intercepted the outbound provider payload:response_format.json_schema.schemacarries"quantity": {"type":"integer","exclusiveMinimum":0,"maximum":2147483647}. The bounds do reach the model, not just the validator.The modal — fully fixed. Drove the real thing in a browser against a scratch row, 11 inputs:
The preview can no longer display an outcome the save cannot reach — it renders only under
canSubmit, and the resulting-stock invariant is mirrored client-side, so the over-removal case is refused with the actual stock in the message.1.500refused /1500accepted is #56's ruling held exactly.inputMode="numeric"with notype="number"removes the accidentalstep="1". This defect is closed properly.Public consequence chain — reproduced end to end against real Postgres and a production server, both branches:
add-to-cart.tsx:38-40disables the cart forhabisonly, soindentkeeps the full flow. The harmful branch is real andadjustStocknow closes its door on it.Two decisions you asked me to judge
Whole-record rejection: right — keep it, and for a stronger reason than the PR gives. Partial-with-warning is the correct default for a bulk import that has a manifest to reconcile against. This has neither. The artifact is a photo the operator will discard, WhatsApp has no durable surface (a "✅ saved, 1 line skipped" line scrolls away within one turn and there is no reconciliation view to catch it later), and
buy_priceis overwriting (set buyPrice = item.buyPrice), not append-only — so a dropped line does not merely omit a cost, it can leave a stale cost basis that reads as authoritative. Rejection is only better if the operator learns which line, anddescribeBotRejectiondoes namebaris N. It holds.StockChangeSchema(field, currentStock, itemLabel): good design. It pulls an invariant that needs two facts into a pure function, so "−99 against 12" is an assertion instead of a fixture, whilelockStockForChangekeeps the row read — and its lock — at the service where it belongs. Correct seam. Its one structural cost is that the schema cannot enforce that its caller locked the row first, which is exactly finding 4.Pre-existing lint: leaving it is right, but leave all three, not two. Since the squash body lands empty,
git blameis the only provenance that survives this merge, and an unsafeuseOptionalChainautofix in the same commit as the invariant muddies it. So don't singlenoUnusedImportsout as a deviation — either all three are drift or none are. If you do fix one,OrderItemRowis the right one: pure deletion, no semantics.Findings
apps/management/lib/db/validators.ts:465: blocker: restating an uncaught server-action error does not defeat Next's production redaction — the mechanism this helper exists for does not work, and four comments assert that it does. Built withnext buildand served withnext start, then drove the modal against a stock changed behind its back (client previewed3 → 0, save enabled, service refused). The toast reads verbatim:An error occurred in the Server Components render. The specific message is omitted in production builds to avoid leaking sensitive details.The restated Indonesian sentence — which does appear innext dev, and that dev run is exactly the false green this produces — never reaches the browser. Redaction applies to any uncaught server-action error; restating the message does not change that it is thrown. #69's acceptance ("a save the service refuses shows a reason") is therefore unmet in the only environment that ships, andbuild-publish.ymldeploys the production container on merge. Fix: return the rejection instead of throwing it. That pattern is already in this repo one directory over —createOrderWithItemsreturns{ok:false, code:"INSUFFICIENT_STOCK", items}andadd-order-modal.tsx:85renders a fully specific description from it, which survives production because it is a return value. MakewithReadableRejectionreturn{ok:false, error}and haveadjustment-modal.tsx:105render it. Note this behaviour matches pre-existing house style (lib/inventory/actions.ts:42also throws andinventory-edit-form.tsx:122also toastserr.message), so nothing regresses — but the comments here claim the problem is solved, and with an empty squash body those comments are the durable record.apps/management/lib/db/validators.test.ts:259: should: the 305 added test lines guard the schemas and nothing guards their placement at the service, which is the entire thesis. Mutation-tested all 19 bounds: 10/10 schema mutations caught (dropping.int()/.max(), neutering eachStockChangeSchemarefine,Int4IdTextSchema,describeZodError's message — 1 to 9 named failures each, all real). 8/8 wiring mutations survived with 280/280 still green: deletingAdjustStockInputSchema.parse(input)fromadjustStock,PurchaseItemsBoundsSchema.parsefrom bothcreatePurchaseandupdatePurchase,OrderItemsBoundsSchema.parsefromcreateOrder, bothlockStockForChangecalls,createOrder's physical-stock guard, and.for("update")from the row lock. A future refactor can delete the bound this PR is about and the suite says fine — which is PR #68's mistake transposed into the tests: the schema is enforced, the wiring is left to review. This does not need Postgres, which is what the stated "unit-tested rather than integration-tested" deviation covers: the.parse()in all three services runs beforetxis touched, soawait expect(createPurchase({} as DbTransaction, {items:[{inventoryId:"1",qty:9_999_999_999,buyPrice:1}]})).rejects.toThrow(ZodError)is three one-line tests with a{}stub. Add those three plus a small faketxforlockStockForChange.apps/management/lib/purchases/service.ts:126: should: a thirdinventory.stockwrite site in this file is unguarded, and it commits a negative.deletePurchasedoesstock - ${item.qty}with nolockStockForChange. Measured, committed, on a live server: buy 10 → sell 10 → delete the purchase →stock = -10, then/api/public/catalogreturnsindentunderallow_indent: true. That is an ordinary correction an operator makes, not a crafted sequence. Identical onorigin/main, so it is pre-existing rather than introduced — but it is the same invariant, in the same file the diff opens to add the guard, andstock-movements/service.ts:39records the invariant as "stated once:adjustStockhere,upsertPurchaseIteminlib/purchases/service", naming exactly the two guarded sites and omitting the two reversal sites. Fix:await lockStockForChange(tx, item.inventoryId, -item.qty, "qty")before each subtraction.apps/management/lib/purchases/service.ts:177: should:updatePurchase's reversal branch is unguarded too, and is currently safe only by accident. With a non-empty new item list the reversal drives stock to −10 unlocked and the re-insert is then caught by the new guard — visibly so, since the message readscurrent stock is -10, a stock that is already negative when the check runs. Withitems: []there is no re-insert to catch it andstock = -10commits (measured). Same one-line fix.apps/management/lib/orders/service.ts:90: should: the new physical-stock guard readsinventory.stockwithout.for("update"), so the exact race its sibling helper exists to prevent is open on this path.lockStockForChange's own docblock says the lock matters "because the check and the write are two statements, so without the row lock two concurrent removals could each see enough stock and together commit a negative" — and this guard reads at L90-97 and writes at L168-171 with no lock. Measured with two concurrent transactions against a stock 5 / lots 10 divergence: both orders of 5 passed the guard on a stale read andinventory.stockcommitted −5. The same race againstadjustStockis correctly refused (B: ZodError would leave "…" at a negative stock; current stock is 0, final stock 0), so the helper is right and one call site is missing the lock. Fix:.for("update")on that select, plus.orderBy(asc(schema.inventory.id))so two orders naming the same items in different sequence cannot deadlock.apps/management/lib/db/validators.ts:461: should: the PR #76 contract holds for MCP and the bot but is broken on REST, and #76 is building against it.withReadableRejection's docblock says "The REST and MCP transports need none of this — they answer from the API's error handler, which renders theZodErrordirectly." REST does not bypass it:app/api/[[...route]]/purchases.ts:53andorders.ts:69call the server actions, which wrap the service in this helper. Measured, same rejection through both doors: service direct →ZodError; viawithReadableRejection→ plainError. Reachable on the real path, because the rejections that reach the service are the ones no transport schema can express —qty: 2147483640passesCreatePurchaseSchemaand trips the resulting-stock invariant, androute.ts:36accepts a session cookie sorequireAuth()inside the action succeeds. So a session-authenticatedPOST /v1/purchaseswill hand #76'sonErroranError, not aZodError, and get a generic 500 where #68's field-named envelope was promised. Theblockerfix above resolves this too if the helper returns instead of throwing; otherwise preserve the original ascauseand tell #76.apps/management/lib/bot/nodes/classify-intent.ts:401: should:describeBotRejectioncannot fire on the path it was added for, and this comment asserts that it does.withStructuredOutputwraps a schema failure in langchain'sOutputParserException, which carries onlyllmOutput/observation— nocause, noissues(checked its own keys). SodescribeBotRejectionreturnsnull, the...(rejected ? {reply: rejected} : {})spread contributes nothing, and the node returns the bareclassifiedIntent: "UNKNOWN"that the comment says it replaces — "Say so instead of answering 'UNKNOWN' with no explanation" is the opposite of what happens. Measured with a stubbed provider answer ofunitPrice: 9999999999:OutputParserException/is ZodError: false/describeBotRejection -> null. Nothing is written and the service bound still holds, so this is message quality, not safety.vision-llm.ts:95hedges this correctly and is fine as written, though its fallback shipsFailed to parse. Text: "{…full JSON…}"to the operator's chat. Fix: re-parseerr.llmOutputwith the schema to recover theZodErrorbefore handing it over, or matchOutputParserExceptionindescribeBotRejection. Then correct theclassify-intent.tscomment either way.apps/management/lib/bot/nodes/execute-order.ts:49: nit:Order #${orderId}interpolates the{ok, id}objectcreateOrderreturns, so the operator's WhatsApp reply readsOrder #[object Object]. Pre-existing onorigin/main, but this diff opens the file and it isorderId.id.apps/management/lib/orders/service.ts:187: nit:OrderStockError's message is still"Insufficient lot stock"but the new guard throws it for a physical count shortage, which is a different quantity.execute-order.ts:57matches onmsg.includes("Insufficient")so nothing breaks; the string is just now slightly untrue in one of its two cases.PR body, Verified section: nit: the third biome warning is
noUnusedImportsonOrderItemRow(orders/service.ts:10:3), not onrecordMovement—recordMovementis used at L172. The substance is correct and I verified it: 3 warnings, all pre-existing, identical rules and selectors onorigin/main, line-shifted only.Everything above ran locally; nothing checked this branch automatically (#73). Not filed, per scope: #77, #70/#76, #73, #72.
Database disclosure. Started Postgres on the existing datadir (it was not running) and stopped it again; nothing else was bound on 5432, 7411 or 7412 when I finished. Wrote only to rows I created: one scratch inventory item (
ZZ Probe78 Oli), two race fixtures (__race78,__race78b), their purchases/orders/lots/movements, and one loginrev78@local.testwith 3 sessions. All deleted. Everything else ran inside savepoints in rolled-back transactions. Baseline rows 4 and 5 were never read as fixtures and never written —4|65000|85000|12and5|25000|40000|8,weight_gramsNULL, both published, unchanged. Final state:md5(string_agg(inventory))=187c8b21e6425ee4489fa22612c1d447, byte-identical to the value I captured before starting;stock_movements/inventory_lots/purchases/purchase_items/orders/order_itemsall 0; migrations 12/12; sessions back to 9;op44@local.test,op59@local.testandsvc71@local.testleft alone. The user'sstash@{0}is untouched andgit status --porcelainon the default checkout shows onlyM .omp/mcp.json.VERDICT: request-changes
Round 2 at
4107ac9. My round-1 blocker is genuinely fixed — verified in a production build, not dev. But the fix introduced a silent-success regression on a transport nobody enumerated, and the new test file does not pin what it claims to pin.Toolchain re-run independently:
vitest run→ 11 files, 289 passed;tsc --noEmitfromapps/management→ exit 0;biome checkon the 13 changed files →Checked 13 files.with noFoundline — genuinely clean, andapp/api/[[...route]]/orders.tspasses bothcheckandformaton its own with nobiome-ignoreanywhere in the branch. Across all 23 files vsorigin/mainthe same 3 pre-existing warnings and no new ones.The blocker fix works
Production build,
next start, real login, stock changed behind the modal's back so the client previewed1 → 0with save enabled:That is the sentence, in the environment that ships, where round 1 gave Next's redaction text. A return value crosses the boundary untouched, exactly as you said. All four UI consumers render it — I read all four and drove one.
And both reversal sites now hold. Review 60's measured harm, re-run against real Postgres:
Your two direct questions
Are all 8 wiring mutations caught? No — three of the original eight still survive, plus two of the new guards.
*
.for("update")is caught incidentally: the stub's select chain only resolves through.for, so removing it yields anitem_missingrejection that fails the message match. Nothing asserts the lock's purpose, so the concurrency property review 60 measured is still undefended — it just happens to be structurally load-bearing in that one stub.Line 11 is the one I would look at first. Reverting your entire blocker fix leaves the suite green —
runActionandActionResulthave zero test coverage (grep runAction --include=*.test.ts→ nothing). The fix for the wave's most consequential finding is the least defended thing in the change.Is the REST unreachable-case handling right? No — the case is reachable, so the premise the design rests on is false.
CreateOrderSchema.items[].inventoryIdis a barez.string(); the service usesInt4IdTextSchema. Measured against the production server:The comment says the route "validates the same numbers at the transport". It validates
quantityandpriceSoldidentically — but notinventoryId, which is the one field where the transport and the service genuinely disagree.CreatePurchaseSchemahas the same gap (z.string().nullable()).Ruling: keep the throw, but make its premise true rather than declaring the response. Bound
inventoryIdwith the sameInt4IdTextSchemathe service uses, in bothCreateOrderSchemaandCreatePurchaseSchema. That makesINVALID_INPUTgenuinely unreachable from REST, gives #68's field-named 400 for free — the control above shows that envelope already works — and is single-sourcing rather than a second rule, which is this PR's own discipline. Declaring the response in OpenAPI would be worse: it documents a state the design asserts cannot occur, and it would have hidden this. #76 does not help either way;onErrorrenders a plainErroras a 500.The tripwire — you were right to ask
It does not pin the field. The assertions are
/Too big|expected|invalid/i, which anyZodErrorsatisfies. I left every schema intact and changed only which schema the two services consult, to one that rejects the tripwire's input for an unrelated reason while leavingqty,buyPrice,quantityandpriceSoldcompletely unbounded at the service:All four
createPurchase/createOrdertripwire tests pass while the services bound nothing numeric — the exact regression the file exists to catch. (An earlier variant of mine was caught, but byvalidators.test.ts, not by the wiring tests: the schema tests noticed, the wiring tests did not.) Fix: assert the issue path, whichvalidators.test.ts:319already does one file over —expect(err.issues[0].path).toEqual(["items", 0, "qty"]).Your tightened
deletePurchaseassertion is sound and I could not make it pass another way./Too small|negative|stock 0|tersisa/iagainst each candidate: guard's negative message → matches; tripwire"transaction touched: .update() ran past the guard"→ no; overflow-branch"would push … past the maximum stock of …; current stock is 0"→ no. And neutering only the negative refine fails it. It is pinned to the branch it names. Good catch on your own draft.Findings
apps/management/app/api/[[...route]]/purchases.ts:53: blocker: the route discards the newActionResult, so every service-layer rejection now answers201 Created {"message":"ok"}with nothing written. Measured against the production server, two reachable ways, both with the four purchase tables snapshotted before and after:The first is reachable because the transport declares
inventoryId: z.string().nullable()while the service usesInt4IdTextSchema; the second because the resulting-stock invariant is not expressible at any transport, which is the whole reason it lives in the service. This is a regression introduced by this round — at583536cthe same inputs threw and produced a 500. A 500 is the wrong shape but it is visibly a failure; a 201 is a lie, and a client that trusts it records a purchase that does not exist.runAction's own docblock names this exact hazard: "Callers must rendermessageonok: false. Ignoring it reproduces the silent failure this replaced." Worth noting why the compiler did not save you here when it did for orders: wideningCreateOrderResultmade every reader ofresult.codea type error, but changingPromise<void>toPromise<ActionResult>is invisible to a caller that doesawait f()and discards the value. Fix:const r = await createPurchaseWithInventory(body); if (!r.ok) return c.json({ error: r.message }, 400);— and declare the 400, since unlike the orders route this one is genuinely reachable untilinventoryIdis bounded at the transport.apps/management/lib/db/service-wiring.test.ts:42: should: the tripwire tests do not pin the field, so a service that stops bounding its numbers entirely still passes them — demonstrated above with 289/289 green whileqty,buyPrice,quantityandpriceSoldwere all unbounded at the service./Too big|expected|invalid/imatches any zod message, including one about a different field. Assert the issue path instead of the message text, the wayvalidators.test.ts:319does. While there:viis imported for avi.restoreAllMocks()that has no mocks to restore.apps/management/lib/db/validators.ts:479: should:runActionandActionResulthave no test, so mutation 11 — reverting the return to a throw, which is the whole blocker fix — leaves 289/289 green. Two lines close it: assertrunAction(() => { throw new ZodError([...]); })resolves to{ ok: false }and that a non-ZodErrorstill rejects. Worth pairing with one assertion per action that a rejection is returned rather than thrown, since that is the property the four UI consumers depend on.apps/management/lib/purchases/service.ts:157: should: three wiring mutations from review 60 still survive with the suite green —updatePurchase'sPurchaseItemsBoundsSchema.parse(mutation 3),upsertPurchaseItem'slockStockForChange(mutation 6), andupdatePurchase's newly-added reversal guard (mutation 10). The new file coverscreatePurchaseanddeletePurchasebut notupdatePurchase, which reaches both the bounds parse and both stock directions. Same tripwire shape as the tests already there.apps/management/lib/orders/service.ts:130: should:createOrder's physical-stock guard is still undefended — mutation 7 deletes it and 289/289 pass. This is the guard whose absence I measured committing a negative stock in round 1, and it is the one guard in the change with no test at all. A stubtxwhose select resolves to[{id:1,name:"x",stock:1}]and whoseinsertis a tripwire covers it without a database.apps/management/lib/orders/service.ts:90: should: carried forward from review 60 and not addressed — the physical-stock guard still readsinventory.stockwithout.for("update")while writing at L168-171. Measured in round 1: two concurrent orders of 5 against a stock 5 / lots 10 divergence both passed on a stale read and committedstock = -5, where the same race againstadjustStockis correctly refused. One word plus.orderBy(asc(schema.inventory.id))for deadlock ordering.apps/management/lib/bot/nodes/classify-intent.ts:401: should: carried forward and not addressed —withStructuredOutputwraps the schema failure inOutputParserException(onlyllmOutput/observation, nocause, noissues), sodescribeBotRejectionreturnsnulland the node returns the bare"UNKNOWN"this comment says it replaces. Safety is unaffected; the comment is the part that is wrong.apps/management/app/(dashboard)/pembelian/purchase-table.tsx:211: nit: the receipt-URL-onlyupdatePurchasecall discards the result. Harmless today — with noitemsthe service neither parses nor reverses, so noZodErroris possible — but it is the same shape as the blocker one file away, and the reason it is safe is not local to the call.apps/management/lib/bot/nodes/execute-order.ts:49: nit: carried forward —Order #${orderId}interpolates the{ok, id}object, so the reply readsOrder #[object Object].apps/management/lib/orders/service.ts:187: nit: carried forward —OrderStockError's message is still"Insufficient lot stock"but is now thrown for a physical-count shortage too.Everything above ran locally; nothing checked this branch automatically (#73). Not filed, per scope: #77, #70/#76, #73, #72.
Database disclosure. Started Postgres on the existing datadir (it was not running) and stopped it; nothing bound on 5432 or 7413 at finish. Committed writes only to rows I created — one scratch item
ZZ R2 Probe, the one purchase and one order the REST control probes made through it, and one loginrev78r2@local.testwith 2 sessions — all deleted. Every service probe ran inside savepoints in a rolled-back transaction. Baseline rows 4 and 5 were never used as fixtures and never written:4|65000|85000|12,5|25000|40000|8,weight_gramsNULL, both published. Finalmd5(string_agg(inventory))=187c8b21e6425ee4489fa22612c1d447, byte-identical to the value captured before round 1 and again before round 2; ledger tables all 0; migrations 12/12; sessions back to 9;op44,op59andsvc71left alone. The user'sstash@{0}is untouched andgit status --porcelainon the default checkout shows onlyM .omp/mcp.json.Review 64: 1 blocker, 5 shoulds. The blocker was a regression I introduced in the previous commit, and it was worse than the fault it replaced. **BLOCKER — `201 Created` on a rejected write.** Widening the purchase action to return `ActionResult` left `app/api/[[...route]]/purchases.ts:53` still doing `await createPurchaseWithInventory(body)` and discarding it, so a service rejection answered `201 Created {"message":"ok"}` with nothing written. The previous behaviour was a 500 — the wrong shape, but honest. A 201 is a lie, and `Promise<void>` → `Promise<ActionResult>` is invisible to a caller that awaits and drops the value, so the compiler could not catch it. The orders path only got caught because widening `CreateOrderResult` made readers of `result.code` type-error. The route now returns the rejection as a declared 400. Unlike the orders route's unreachable case, this one is genuinely reachable: a `qty` overflowing the row's RESULTING stock is a fact about the stored row, which no transport can express. **A second discard of the same class, not in the review.** Auditing every `ActionResult` caller found `purchase-table.tsx:211` dropping the result of the receipt-URL update AND then setting local state as if it had succeeded — so a refused change displayed the new URLs over an unchanged row. It bails before touching state now. **The REST comment's premise was false.** The route claimed the transport validates the same numbers; it bounded `quantity` and `priceSold` but not `inventoryId`, the one field where transport (`z.string()`) and service (`Int4IdTextSchema`) genuinely disagreed — so `"abc"` reached the throw and surfaced as a 500. Both `CreateOrderSchema` and `CreatePurchaseSchema` now use the existing `Int4IdTextSchema`, which makes the premise true, yields #68's field-named 400 for free, and single-sources the digit rule. Per the reviewer's ruling the response is NOT declared in OpenAPI: that would document a state the design says cannot occur, and would have hidden this. That move had a hazard the typechecker cannot see: `Int4IdTextSchema` was declared *below* both schemas, and these object literals evaluate at module load, so referencing it in place would have been a TDZ `ReferenceError` on import. The declaration is hoisted above its use sites and a test asserts the module loads and rejects by field path. **The wiring tests passed for the wrong reason.** Review 64 left every schema intact and changed only WHICH schema the services consult, to one rejecting an unrelated field while leaving `qty`/`buyPrice`/`quantity`/`priceSold` unbounded at the service — and all tests passed, because `/Too big|expected|invalid/i` matches any `ZodError`. A message match is not evidence. Every assertion now names the issue **path**, and the stored-row guards also assert the `RejectionReason`, which tells "would go negative" apart from "would overflow" and "no such item". A non-`ZodError` — including the tripwire's own — is rethrown rather than laundered into a pass. **Guards that had no test at all** now have one: `updatePurchase`'s bounds parse, `upsertPurchaseItem`'s `lockStockForChange`, `createOrder`'s physical-stock guard, `updatePurchase`'s reversal guard, and `runAction` itself — reverting the entire blocker fix used to leave the suite green. Since the deeper guards run after a row is read, a tripwire that throws on every method cannot reach them; a second shape lets reads resolve and keeps every write a tripwire, so the assertion is still "it refused before writing". Full battery, all now caught (each mutation applied and the file diffed to confirm it landed — two earlier "survivals" were regexes silently not matching, which is why every mutation below was verified applied): adjustStock schema parse / lock caught createPurchase bounds parse / upsert lock caught updatePurchase bounds parse / reversal guard caught createOrder bounds parse / physical guard caught deletePurchase reversal guard caught runAction returning -> throwing caught stock-movements .for("update") caught orders FIFO lot .for("update") caught review 64's unrelated-field schema swap (x2) caught The last is one the review did not name: auditing for siblings of the lock it did name turned up a second unguarded locking read, the FIFO lot select in `depleteLotsAndComputeCogs`. Losing that one is the more dangerous, since two concurrent orders would each read the same lots as available and both deplete them. ponytail: both lock tests assert the clause is PRESENT, not that it serialises, and the comments name that ceiling. A real lost-update race needs two connections against real Postgres; a stub cannot exhibit one. Upgrade path is recorded at the tests rather than implied. 301 tests, up from 289. tsc exit 0, biome clean on all 5 changed files.VERDICT: request-changes
Round 3 at
c2d4917. The round-2 blocker is genuinely fixed on both doors, the test file now survives my attack, and theActionResultaudit is complete. Two things remain: a race I have now measured in three consecutive rounds and which this change believes it closed, and one reachable throw left on the pathrunActionwas built to own.Re-run independently:
vitest run→ 11 files, 301 passed;tsc --noEmitfromapps/management→ exit 0;biome checkon the 5 changed files →Checked 5 files.with noFoundline, genuinely clean; full 24-file branch vsorigin/main→ the same 3 pre-existing warnings, zero new.lib/orders/service.ts,lib/purchases/service.tsandlib/stock-movements/service.tsare byte-identical to4107ac9— confirmed.The blocker is fixed, both doors, measured in production
The 201-lie is gone, and
nullstill means "new item":inventoryId: nullwithnewItem→ 201, and it wrote the new inventory row plus purchase, lot and movement, leaving the referenced row's stock alone. The asymmetry between the two routes is now correct and I want to be explicit that it is: for orders, the transport is a strict superset ofOrderItemsBoundsSchemaon every field, and the physical-count guard throwsOrderStockError→ 409, soINVALID_INPUTreally is unreachable and the loud throw is a proper tripwire. For purchases, the schema rejections are likewise unreachable now, butStockChangeSchemareads the stored row and no transport can express that — so a declared 400 is right there. Same reasoning, opposite conclusion, both correct.The mutation battery: 15 of 16 caught
Every mutation diffed after applying, so a regex that failed to match cannot report a meaningless green.
My attack is dead. Leaving every schema intact and swapping only which schema the two services consult — to one rejecting an unrelated field while
qty/buyPrice/quantity/priceSoldare unbounded at the service — now produces 7 failures where it produced 0. Asserting the issue path plus theRejectionReasonis what did it, and rethrowing a non-ZodErrorinrejectionIssuesis what stops the tripwire laundering itself into a pass. That is the right shape.Your four questions
Is the
ActionResultaudit complete? Yes. Six producers, seven call sites, and all seven both capture the result and branch onresult.ok:lib/mcp/tools/purchases.ts:152calls the service, not the action, so it is correctly outside this set. You found both discard sites;purchase-table.tsx:211setting local state as if the write had succeeded was the worse of the two and I had under-rated it as a nit. But the audit closed the wrong boundary by one step — see thepurchases/service.ts:33finding:runActionconvertsZodErroronly, and there is still a reachable plainthrowinside the service it wraps.Is there a third unguarded locking read? No — and there never was a second. Exhaustive sweep: every mutating statement in
lib/andapp/(39 sites), every.for(clause, and every read that determines a subsequent write. The stock/ledger domain has exactly three determining reads —lockStockForChange(locked), the FIFO lot select (locked), andcreateOrder's physical-count read (not locked). Everything else is either a check-then-act backed by a database constraint —inventoryNameConflictbyinventory_item_name_uniq/inventory_slug_uniq, the webhook dedup by theprocessed_webhook_eventsprimary key with the insert already wrapped in try/return,whatsapp_identitiesby itsphone_numberprimary key — or does not compute its write from its read. Nothing needs a fourth lock.One correction, since you asked me to press on it: the FIFO lot select was never unguarded.
git show origin/main:…/orders/service.ts | grep -F 'for("update")'→ 1 occurrence, and the same at9e55770,583536c,4107ac9andc2d4917. The clause has been there the whole time; what you found and closed was that it was untested, which is real and worth having. It was a second untested lock, not a second unguarded read — and I did name it in review 60 when I quotedlockStockForChange's docblock pointing at it. Worth stating because with an empty squash body the comment atservice-wiring.test.ts:335is the durable record, and "auditing for its siblings turned up a SECOND locking read with no coverage" reads as if the lock was added here. (My own method failed on this first:grep -c 'for("update")'returned 0 for every revision because of the parentheses, and I nearly reported the lock as absent.-Ffixed it.)My call on shape-versus-behaviour lock tests: keep the shape assertions. Do not write the two-connection test. Four reasons, and the fourth is the one that decides it. The realistic regression is a refactor deleting the clause, and a shape assertion catches exactly that — mutations 10 and 11 both fail. A two-connection test would assert Postgres's own
FOR UPDATEsemantics rather than this codebase's logic, which is the wrong unit. There is no database-backed test infrastructure here at all: all 11 files are pure unit tests,lib/db/index.tsbuilds apg.Poolat module scope — the documented reason the bounds live invalidators.ts— and no CI runs the tests (#73), so a test needing a live Postgres would fail on any checkout without the datadir up and nothing would notice. And the behavioural evidence already exists and is durable: I have measured both locks by hand three rounds running, and it is in this PR's review record. Theponytail:marker puts the ceiling in the debt ledger, which is its right home. Your judgement was correct.The pairing matters though: a shape assertion is only worth something where a clause exists. The one read that needs behaviour is the one with no clause at all, which is the next finding — there the answer is not a better test.
Findings
apps/management/lib/orders/service.ts:89: should: the physical-count guard still readsinventory.stockwith a plainselectand writes at L168-171, so the race remains open and I have now measured it at three consecutive heads.lib/orders/service.tsis byte-identical to4107ac9, so this should of review 64 was not actioned — I mention that only because the round-3 message says all five were. Re-measured atc2d4917, two connections, divergence fixturestock 5 / sum(qty_remaining) 10, which any manual adjustment after a purchase produces:Same code, same fixture, same run: the locked sibling refuses, this one commits a negative. That is the last path that can reach the publicly-orderable
indentstate #69's added scope exists to close, and the acceptance criterion this change extended tocreateOrder— no caller commits a negativeinventory.stock— is measurably false on it. Not a regression: before this PRcreateOrdernever read the column at all and drove it negative unconditionally, so single-threaded behaviour is strictly better. But this is the third round it has been measured. Fix is one clause on that select plus.orderBy(asc(schema.inventory.id))so two orders naming the same items in different sequence cannot deadlock, and then a third shape assertion alongside the two you added.apps/management/lib/purchases/service.ts:33: should: a reachable plainthrow new Error(conflict)sits inside the servicerunActionwraps, so an ordinary operator mistake still produces the failure this whole mechanism was built to eliminate.runActionconvertsZodErroronly and rethrows everything else by design — correct in general, butinventoryNameConflictthrows a plainErrorfrom insideupsertPurchaseItem, whichcreatePurchasereaches whenever a line carriesnewItem. Measured against the production server:On the dashboard the same throw is redacted in production, exactly as review 60 measured — so "Nama barang sudah ada" reaches the operator as the generic Server Components message. It is reachable from the bot too:
execute-purchase.tsbuildsnewItemfrom afindInventoryByPrefixmiss, so a near-miss name lands here. The conflict message is already operator-ready prose; it just needs to travel as a return. Simplest fix consistent with what you built: haverunActionalso catch a tagged conflict, or raise the conflict as aZodErroron thenewItem.item_namepath so it flows through the machinery already in place and inherits the 400. Notelib/inventory/actions.ts:42/71/140throw the same way, but those are a separate pre-existing surface this PR does not own — worth a follow-up issue, not a change here.apps/management/app/api/[[...route]]/purchases.ts:64: should: the blocker's own fix is the one thing in the change with no test — mutation 13 reverts the route to discarding the result and 301/301 stay green. This is the second time a discardedActionResulthas shipped in this PR, so the class is demonstrated rather than hypothetical, and a comment is the only thing currently standing between it and a third. Importing the router pullslib/dband its module-scope pool, so a route test is genuinely awkward — which argues for moving the mapping into a tiny pure helper (rejectionResponse(result)returning{ status, body }) that is unit-tested, leaving one line of untested wiring instead of the whole decision.apps/management/lib/bot/nodes/classify-intent.ts:401: should: carried from reviews 60 and 64, still unaddressed.withStructuredOutputwraps the schema failure inOutputParserException, which carries onlyllmOutput/observation— nocause, noissues, verified from its own keys — sodescribeBotRejectionreturnsnull, the spread contributes nothing, and the node returns the bareclassifiedIntent: "UNKNOWN"that this comment says it replaces. Safety is unaffected; the comment is the wrong part. Re-parseerr.llmOutputwith the schema to recover theZodError, or matchOutputParserExceptionindescribeBotRejection.apps/management/lib/db/service-wiring.test.ts:159: nit:tscdoes catch the TDZ hazard, so the stated reason for the module-load test is wrong. MovingInt4IdTextSchemaback below its use sites giveslib/db/validators.ts(197,20): error TS2448: Block-scoped variable 'Int4IdTextSchema' used before its declarationplusTS2454, andtsc --noEmitexits 1 — alongside the runtimeReferenceError: Cannot access 'Int4IdTextSchema' before initialization. The hoist is right and the test is harmless belt-and-braces; only "tscpasses either way" is false, and it would teach the next reader that the typechecker is blind to declaration order here. The current import order is genuinely safe: the module loads, and the only value the hoisted schema closes over is the importedparseInt4Input—RejectionReasonis a type and erases, sosatisfiescannot create a cycle.apps/management/lib/db/service-wiring.test.ts:174: nit: the valid-input control coversadjustStockonly. It is the assertion that stops every other test passing on a service that rejects everything, andcreatePurchase,updatePurchaseandcreateOrderdo not have one — so a bound narrowed to reject all input at the asserted path (sayqty: …max(0)) would still pass. Much narrower than the hole review 64 found, and cheap to close with one legitimate-input case per service.apps/management/lib/bot/nodes/execute-order.ts:49: nit: carried —Order #${orderId}interpolates the{ok, id}object, so the reply readsOrder #[object Object].apps/management/lib/orders/service.ts:187: nit: carried —OrderStockError's message is still"Insufficient lot stock"but is now thrown for a physical-count shortage too.Everything above ran locally; nothing checked this branch automatically (#73). Not filed, per scope: #77, #70/#76, #73, #72.
Database disclosure. Started Postgres on the existing datadir (it was not running) and stopped it; nothing bound on 5432 or 7414 at finish. Committed writes only to rows I created — scratch items
ZZ R3 ProbeandZZ R3 Fresh, two__rc3_*race fixtures, the purchases/orders/lots/movements the REST control probes made through them, and one loginrev78r3@local.testwith 1 session — all deleted. Baseline rows 4 and 5 were never used as fixtures and never written:4|65000|85000|12,5|25000|40000|8,weight_gramsNULL, both published. Finalmd5(string_agg(inventory))=187c8b21e6425ee4489fa22612c1d447, byte-identical to the value captured before rounds 1, 2 and 3; ledger tables all 0; migrations 12/12; sessions back to 9;op44,op59andsvc71left alone. The user'sstash@{0}is untouched andgit status --porcelainon the default checkout shows onlyM .omp/mcp.json.Review 65: 0 blockers, 4 shoulds, 4 nits. The blocker from review 64 was confirmed fixed in production on both doors, and the route asymmetry was confirmed correct in both directions — declared 400 for purchases because StockChangeSchema reads the stored row, unreachable-and-throw for orders because the transport is now a strict superset. **The physical-count read was the last unlocked determining read.** `createOrder` read `inventory.stock` to decide its guard without locking it, so it was check-then-act. Measured across three review rounds, two connections, stock 5 and lots 10: both orders of 5 passed their own guard and committed, leaving `inventory.stock = -5`, while `adjustStock`'s locked equivalent refused the second and landed on 0. A negative count is the last remaining path to the publicly-orderable `indent` state this PR exists to close, so the criterion the change extended to `createOrder` was measurably false on exactly this line. Now `.orderBy(asc(inventory.id)).for("update")`. The ordering is not cosmetic: two orders naming the same items in opposite sequence would otherwise take the row locks in opposite sequence and deadlock. The reviewer swept all 39 mutating statements and confirms this closes it — exactly three determining reads in the stock domain, all now locked; everything else is check-then-act backed by a constraint (unique indexes, the `processed_webhook_events` PK, `whatsapp_identities`' PK). **A readable rejection was answering 500.** `inventoryNameConflict` produces a message already written for an operator, but the service threw it as a plain `Error`, and `runAction` converted only `ZodError` — so it was rethrown as an unexpected fault: HTTP 500 instead of the declared 400, and redacted by Next on the dashboard. Reachable from the WhatsApp bot too, since `execute-purchase` builds `newItem` whenever `findInventoryByPrefix` misses. The distinction `runAction` needs is "did the service decide this, or did something break", and `ZodError` had been standing in for the whole first half. A `ServiceRejection` class names it. A plain `Error` is still rethrown, so a genuine fault reaches the logs as itself. **The blocker's own fix was untested** (mutation 13): reverting the route to discarding the result stayed 301/301 green, which is how the regression got in. Rather than extract a helper, `purchases.test.ts` exercises the real route with only the action stubbed — routing, the OpenAPI request schema and the response mapping are real, since the mapping is trivial and the WIRING was the missing part. It also pins the `inventoryId` bound end to end: `"abc"` is a field-named 400 and the action is never reached, `null` still means a new item. Round-3 battery, every mutation diffed after applying: 13 route discards ActionResult again caught (was green) 14 inventory read unlocked again caught, 3 tests 15 conflict thrown as a plain Error caught (green until a service-level test existed; the runAction unit test alone did not reach it) 16 runAction drops ServiceRejection caught **Two corrections to my own record**, both of which the reviewer caught and both of which were mine to make: The FIFO lot select was **never unguarded**. `.for("update")` is present at every revision of this branch and on `main`, and review 60 had already named it. What I actually found was that it was UNTESTED — real, worth having, but a second untested lock rather than a second unguarded read, and the previous comment read as if the lock were added here. Root cause of my error is worth recording: `grep -c 'for("update")'` returns 0 against every revision because of the parens, and I read a broken instrument as absence. `-F` fixes it. The same mistake nearly made the reviewer report the lock as missing. `tsc` is **not** blind to the TDZ hazard. Moving `Int4IdTextSchema` back below its use sites gives TS2448 + TS2454 and exit 2, alongside the runtime `ReferenceError` — verified by doing it. My previous message claimed the typechecker passed either way, which would have taught the next reader to distrust a check that works. The hoist is now documented as enforced. Per the reviewer's ruling the two-connection lock test is deliberately NOT written: the realistic regression is deleting the clause, which the shape assertions catch; a concurrency test would assert Postgres's own `FOR UPDATE` semantics rather than this codebase's logic; and there is no DB-backed test infrastructure here, so it would fail on any checkout. The ceiling stays in the `ponytail:` marker. 310 tests, up from 301. tsc exit 0. biome clean on all changed files; the one warning in `orders/service.ts` is the pre-existing unused `OrderItemRow` import, unchanged and not mine.VERDICT: request-changes
Round 4 at
722919e. The race is closed, the deadlock you asked me to attack does not happen on the orders path, and the 500 is gone. But the deadlock you correctly prevented oncreateOrderis live oncreatePurchase, where the locks this PR added take one row at a time in input order — measured, SQLSTATE40P01, on all three purchase write paths.vitest run→ 13 files, 310 passed;tsc --noEmitfromapps/management→ exit 0;biome checkacross all 26 branch files vsorigin/main→ the same 3 pre-existing warnings, zero new.The race is closed
Re-run at
722919e, two connections,stock 5 / sum(qty_remaining) 10:B blocked on the lock, re-read the committed
stock = 0, and refused. Three rounds of-5become0. That is the fix.And the
orderByis load-bearing, not cosmetic — I grounded that rather than trusting it.EXPLAIN (costs off)on the real statement:LockRowssits aboveSort, so rows are locked in sorted order, which is what makes the idiom work. Attacked as asked —createOrder([A,B])againstcreateOrder([B,A]), concurrently, six rounds:ok | okevery time. No deadlock traded for the old defect on that path.The
ServiceRejectionfix worksBot path verified through the real
executePurchaseNode, not a stub — reply❌ Gagal menyimpan pembelian: Barang dengan nama serupa sudah ada: "…", and nothing written. See the nit below though: it is reachable, but not for the reason both comments give.Your four questions
Is the race closed with no deadlock introduced? On
createOrder, yes — measured both directions above. OncreatePurchase, no: see the blocker.Does the
ServiceRejectionconversion have a mutation-15 wiring gap? Yes, and it is the same gap one layer up. Nothing asserts that the actions callrunAction:The service throws correctly (mutation 17 caught),
runActionconverts correctly (18 caught), the route maps correctly (13 caught), the UI renders correctly. The composition is tested layer by layer and never joined, so removing the wrapper from all three actions — which is the whole delivery mechanism for bothZodErrorandServiceRejection— changes nothing. Exactly your mutation-15 shape: a unit test standing in for wiring.vi.mockofdb,requireAuthandrequestStoreDeploycloses it, and the pattern is already inpurchases.test.tsandname-conflict.test.ts.Was my sweep scoped to the stock domain? No — it was all 39 mutating statements in
lib/andapp/, and here is what the non-stock half contains, since you are relying on it. A "determining read" means the write's value is computed from a prior read of the same row, which is what a lock protects; a read that merely gates existence is not one.inventory/actions.ts:92/150andmcp/tools/inventory.ts:115/224computepriceUpdatedAtfromfields.sell_price— the caller's input, not a read of the row..set(updates)writes input values throughout. Nothing to serialise.execute-order-update.ts:19-30readsorders.idfor existence, then writes the inputnewStatus;orders/actions.ts:57,mcp/tools/orders.ts:178and the customer writes are the same shape. A concurrent delete between read and write makes the update match zero rows — a lost update, but it cannot write a wrong value, and there is no arithmetic on the column. That is the one borderline case in the sweep and I am calling it out rather than silently counting it in.inventoryNameConflictat four sites byinventory_item_name_uniq/inventory_slug_uniq; webhook dedup by theprocessed_webhook_eventsprimary key with the insert already wrappedtry { … } catch { return };whatsapp_identitiesby itsphone_numberprimary key.So: three determining reads, all in the stock domain, and nothing outside it needs a lock. That claim is safe to lean on.
Route test versus extracted helper: you were right, no overrule. Mutation 13 is caught now, and it is caught against real routing and a real OpenAPI request schema. The pure helper would have tested the trivial half and left the wiring — which was the missing part — still unasserted, which is the mistake this PR keeps rediscovering. Keep
purchases.test.ts.Both corrections landed
The FIFO note now says plainly that the lock was always present and that what you found was the absence of a test, with the broken-instrument root cause recorded — including that
grep -c 'for("update")'returns 0 against every revision, which nearly cost me the same wrong conclusion. The TDZ note is corrected at the declaration and I re-verified the numbers: moving it back givesTS2448+TS2454and a non-zero exit alongside the runtimeReferenceError. Recording that the check works, rather than that it is blind, is the right call.Battery at
722919e, every mutation diffed after applying: 18 of 22 caught. All 14 previously-caught still caught; 13 now caught; this round's 15, 17, 18 caught; review 64's unrelated-field attack now produces 10 failures. The four survivors are 16 and 19–21, below.Findings
apps/management/lib/purchases/service.ts:44: blocker:upsertPurchaseItemtakes oneSELECT … FOR UPDATEper line, in the caller's line order, so two concurrent purchase writes touching the same items in different order deadlock. This is the defect you prevented oncreateOrderwith.orderBy(asc(inventory.id)), still live on the path where the locks were introduced, and it surfaces as a rawFailed query: … deadlock detected— the exact class #71 exists to eliminate. Measured, and strictly order-dependent:It is not confined to
createPurchase. The reversal loops added in round 2 iteratepurchase_itemsfrom a select whose plan isSeq Scan on purchase_items / Filter: (purchase_id = …)with noORDER BY, so their lock order is whatever the heap returns:Two reversals can therefore deadlock even when the lines were entered in the same order. Reachability is ordinary rather than adversarial: the dashboard orders lines by the sequence an operator picked them, and the bot orders them by the sequence the vision model read the receipt, so two purchases sharing two items in different order is the normal case, not a contrived one. Nothing before this PR held these locks, so this is a new defect rather than an exposed one. Fix is the one already applied a file away — acquire all the line locks in a single ordered statement before the loop, the way
createOrdernow does, or sort the resolved ids ascending before iterating; and add.orderBy(asc(schema.purchaseItems.inventoryId))to the two selects that drive the reversal loops. Worth a shape assertion afterwards, since mutation 16 shows an unordered lock is currently invisible to the suite.apps/management/lib/purchases/actions.ts:24: should: nothing asserts that any action callsrunAction, so the entire delivery mechanism for both conversions can be removed with the suite green — mutations 19, 20 and 21 above, one per action. This is the mutation-15 shape one layer up and it is the third time this PR has found a unit test standing in for wiring, so it is worth closing as a class rather than a case: mockdb.transactionto reject with aServiceRejectionand with aZodError, and assert each action resolves to{ ok: false, message }rather than throwing. Three tests, andname-conflict.test.tsalready establishes the mocking pattern.apps/management/lib/bot/nodes/classify-intent.ts:401: should: carried from reviews 60, 64 and 65, still unaddressed.withStructuredOutputwraps the schema failure inOutputParserException, which carries onlyllmOutput/observation— nocause, noissues— sodescribeBotRejectionreturnsnull, the spread contributes nothing, and the node returns the bareclassifiedIntent: "UNKNOWN"this comment says it replaces. Safety is unaffected; the comment is the wrong part.apps/management/lib/orders/service.ts:108: nit:.orderBy(asc(schema.inventory.id))has no test — dropping only the ordering while keeping.for("update")leaves 310/310 green (mutation 16). It is genuinely load-bearing, per theLockRows-above-Sortplan and the six clean rounds above, and the blocker is what its absence looks like. AsortArgs-style assertion alongside the existingforArgsone costs a line.apps/management/lib/purchases/service.ts:36: nit: the stated bot reachability for the name conflict is wrong, and the same sentence is invalidators.ts:503. AfindInventoryByPrefixmiss does not imply no exact-name collision — it implies the opposite, becauseinventoryNameConflictmatching means a row equals the name, and that row therefore containsname.slice(0, 8)as a substring. Measured with the real functions:The genuinely reachable case is the
btrim-versus-slice(0, 8)asymmetry:inventoryNameConflicttrims,findInventoryByPrefixdoes not, so a name whose first eight characters are whitespace misses the prefix search and still collides. I reached it through the real node and the fix behaves correctly there — readable message, nothing written. The REST route is the honestly reachable transport and is enough to justify the change on its own; the comment just points at the wrong door.apps/management/lib/bot/nodes/rejection.ts:97: nit:describeBotRejectionhandlesZodErroronly, so aServiceRejectionfalls through toexecute-purchase.ts's❌ Gagal menyimpan pembelian: ${message}and loses theTidak ada data yang tersimpan sama sekali.line. That reassurance is the whole point of the whole-record-rejection decision — the operator needs to know the receipt is not half-saved — and a conflict is exactly a case where they would wonder. Oneinstanceofbranch.apps/management/lib/bot/nodes/execute-order.ts:49: nit: carried —Order #${orderId}interpolates the{ok, id}object, so the reply readsOrder #[object Object].apps/management/lib/orders/service.ts:201: nit: carried —OrderStockError's message is still"Insufficient lot stock"but is thrown for a physical-count shortage too.Out of scope, flagged not filed.
findInventoryByPrefixmatchesilike '%' || name.slice(0,8) || '%'withlimit 1and noORDER BY, so the bot books a purchase against an arbitrary substring-matching row: I measured"Oli Mesin 20W-50 BEDA"resolving to"Oli Mesin 10W-40 PROBE", which would add that quantity to the wrong item and overwrite itsbuy_pricewith the wrong cost. Untouched by this PR and outside #71, but it is the same cost-column poisoning #71 was filed about and rather more likely than an OCR magnitude error — worth its own issue.Everything above ran locally; nothing checked this branch automatically (#73). Not filed, per scope: #77, #70/#76, #73, #72.
Database disclosure. Started Postgres on the existing datadir (it was not running) and stopped it; nothing bound on 5432 or 7415 at finish. Committed writes only to rows I created — scratch items
ZZ R4 Probe,ZZ R4 Brand New,Oli Mesin 10W-40 PROBE,ZZ Bot Conflict Probe, the__dl_*/__d2_*/__d3_*deadlock fixtures, the purchases/orders/lots/movements made through them, and one loginrev78r4@local.test— all deleted. Baseline rows 4 and 5 were never used as fixtures and never written:4|65000|85000|12,5|25000|40000|8,weight_gramsNULL, both published. Finalmd5(string_agg(inventory))=187c8b21e6425ee4489fa22612c1d447, byte-identical to the value captured before rounds 1, 2, 3 and 4; ledger tables all 0; migrations 12/12; sessions back to 9;op44,op59andsvc71left alone. The user'sstash@{0}is untouched andgit status --porcelainon the default checkout shows onlyM .omp/mcp.json.Review 67: 1 blocker, 2 shoulds, 5 nits. The race the previous round was filed against is CLOSED — two connections, stock 5 / lots 10, B blocked on the lock, re-read 0, refused with `OrderStockError`, final `inventory.stock = 0` where three earlier rounds measured -5. No deadlock on `createOrder`, grounded rather than assumed: `EXPLAIN` shows `LockRows` ABOVE `Sort`, so the rows really are locked in sorted order and the `orderBy` is load-bearing. **BLOCKER — I introduced the same defect I had just prevented, on the sibling path.** Adding `lockStockForChange` to `upsertPurchaseItem` made the purchase paths take one `SELECT … FOR UPDATE` per line, in the caller's line order: createPurchase [A,B] vs [B,A] -> SQLSTATE 40P01, 6 of 6 rounds same order, both ways -> ok | ok deletePurchase(p1) vs (p2) -> 40P01 deletePurchase vs createPurchase-> 40P01 It surfaced as a raw `Failed query: … deadlock detected` — precisely the unreadable database error #71 exists to eliminate. A NEW defect, not an exposed one: nothing held these locks before this PR. The reversal loops were worse, since their `purchase_items` select is a Seq Scan with no inherent order, so two reversals could deadlock even with every line entered in the same order. Fixed with the shape already a file away: `lockInventoryRows` takes every lock a multi-line write needs, ascending, in ONE statement before the per-line work. The later per-line locks then become re-entrant, so line order stops mattering downstream instead of at each call site. `updatePurchase` locks its old and new lines together, because two passes would still deadlock against a concurrent update whose old set overlaps this new set. Both reversal-driving selects are now ordered by `inventoryId`. **The delivery chain was tested link by link and never joined.** Nothing asserted the actions CALL `runAction`: bypassing it in `purchases/actions.ts`, `stock-movements/actions.ts` and `orders/actions.ts` each left 310/310 green, so the whole mechanism for both conversions was silently removable while every part stayed pinned. Third time this PR has found a unit test standing in for wiring, so `action-wiring.test.ts` closes it as a class: a rejection raised by the service must arrive at the caller as `{ ok: false, message }` through the real action, with only the database mocked. A genuine fault must still propagate. **Two corrections to my own claims, both the reviewer's catch.** My bot reachability argument was backwards. I wrote that a conflict is reachable "whenever `findInventoryByPrefix` misses" — a miss implies the OPPOSITE, since a conflicting row equals the name and therefore contains its first 8 characters, so the prefix would have HIT and `newItem` would never have been built. Measured: exact name -> hit -> unreachable. The real bot door is the `btrim`-versus- `slice(0, 8)` asymmetry, which the reviewer reached through the live `executePurchaseNode`. The REST route justifies the change on its own; three comments pointed at the wrong door and now say so. `describeBotRejection` handled `ZodError` only, so a `ServiceRejection` fell through to `null` and the bot lost "Tidak ada data yang tersimpan sama sekali" — the reassurance the whole-record-rejection decision is built on, and a duplicate item name is exactly when an operator would wonder whether half the receipt went in. Round-4 battery, every mutation diffed after applying: 19 createPurchase pre-lock dropped caught 20 reversal orderBy dropped caught, 2 tests 21 pre-lock ordering dropped caught, 4 tests 22 stock-movements action bypasses runAction caught (was green) 23 purchases actions bypass runAction caught (was green) 24 bot drops ServiceRejection handling caught **Filed rather than fixed: #81.** The reviewer flagged, out of scope, that `findInventoryByPrefix` does `ilike '%first-8-chars%' limit 1` with no `ORDER BY`, so the bot books purchases against an arbitrary substring match — measured, "Oli Mesin 20W-50 BEDA" resolving to "Oli Mesin 10W-40 PROBE", adding qty to the wrong item and overwriting its `buy_price`. Same cost-column poisoning #71 was filed about and likelier than an OCR error, since it needs no misread at all. Every bound in this PR passes, because it writes a valid number to the wrong row — a different defect, so it gets its own issue. Confirmed and not re-derived: the reviewer's sweep covers all 39 mutating statements in `lib/` and `app/`, not just the stock domain. Pricing has no determining read (`priceUpdatedAt` derives from input), status transitions are gated not determined, and all check-then-act is constraint-backed. Three determining reads exist, all now locked. Also confirmed: testing the real route beat extracting a pure helper — mutation 13 is caught against real routing and a real OpenAPI schema, where the helper would have tested the trivial half and left the wiring unasserted. 318 tests, up from 310. tsc exit 0. biome over the PR's 26 changed files: 3 warnings, all pre-existing on `origin/main` (`useOptionalChain` x2, `noUnusedImports`), none on a line this PR touches.VERDICT: approve-with-nits
Round 5 at
ce27c0b. The 40P01 matrix is clean on every pairing that this PR made possible, including all three cases you named, andlockInventoryRowsdoes lock in sorted order — I checked the plan rather than trusting the clause. The wiring class is closed where it can be. What is left is one genuinely undefended link and a set of consumer links that this repo has no way to test, neither of which should hold the merge.vitest run→ 14 files, 318 passed;tsc --noEmitfromapps/management→ exit 0;biome checkover the PR's 27 changed files → 3 warnings, and I confirmed all three sit at line numbers the diff does not touch (execute-order.ts:11,execute-stock-adjust.ts:10,orders/service.ts:10) and reproduce onorigin/main. Scoping that claim was the right call.The 40P01 matrix is clean
Every pairing, run against real Postgres on two connections, opposite line order throughout:
Round 4's 6-of-6 deadlock is 0 of 4 on the same fixture.
updatePurchaselocking old and new lines in one statement is the right call and it holds under partial overlap, which was the case you reasoned about — I measured both the overlapping and the fully-swapped variants.Your (2), new-item lines, is safe and for a reason worth recording: the pre-lock runs before the loop regardless of where the
nullline sits initems, so a new row's id — which only exists inside the loop — can never be requested before an existing row's lock. And a row this transaction just inserted cannot be lock-contended by another transaction, because it is not yet visible. The pairing came backServiceRejection|ok, which is the correct answer rather than a lucky one.lockInventoryRowslocks in sorted order — verified, not assumed.inArray+orderBy+FOR UPDATEis indeed a different plan shape fromcreateOrder's, so I ran it:LockRowsaboveSortin both the narrow and widerINforms, so rows are locked in ascending id order rather than merely returned in it. The idiom is real here too.The wiring class is closed where it can be
Every round-4 fix is defended, and I re-derived it rather than taking the list:
orders/actions.tsis not a third instance, even though no test covers it — I checked before filing it. Both the plain removal and the fair analogue of your mutation 23 failtscwith exit 2 (TS2339: Property 'value' does not exist on type 'ActionResult<CreateOrderResult>'). WideningCreateOrderResultinto a discriminated union made the compiler the guard there, which is stronger than a test because it cannot be skipped. Same for the orders route'sINVALID_INPUTthrow: removing it istscexit 2. Worth knowing so nobody "fixes" those with tests they do not need.Is there a fourth instance? Yes — three links, and one of them is the bot
I mutated every remaining consumer in the delivery chain and checked
tscas well as the suite, so the answer separates "undefended" from "type-guarded":The two bot nodes are the exact shape that has cost this PR four rounds:
rejection.test.tspinsdescribeBotRejectionexhaustively — your mutation 10 fails as soon as theServiceRejectionhandling goes — and nothing proves either node calls it. Delete the call and the guard, keep the fallback, and 318 tests pass with a clean typecheck. The bot is the path #71 was filed for, and "the operator is told, and told that nothing was saved" is the recorded product decision the whole change rests on. It is cheap to close: both nodes are plainasync (state: BotState)functions, and I droveexecutePurchaseNodedirectly in round 4 with only the database live.The four UI consumers are a different problem and I am not going to ask you to solve it here.
vitest.config.tssetsenvironment: "node"and there is nojsdom,happy-domortesting-libraryanywhere inpackage.json, so asserting that a modal rendersresult.messagemeans introducing a DOM test environment — real scope, and not this PR's. The evidence that those four links work is that I drove them in production builds across four rounds. Worth its own issue rather than a finding.#81
Accurate, and the argument for keeping it out of #78 is right: it writes a valid number to the wrong row, so every bound here passes. I have commented one correction on it — the fixture block reads as though both rows existed and the bot was handed one of their names, whereas in my run only the 10W-40 row existed and the incoming
"Oli Mesin 20W-50 BEDA"resolved onto it. That matters because the measured case needs no ambiguity at all (one matching row is enough), and it additionally means the new product is never created, so the old row accumulates both products' stock. The multi-match non-determinism in "Two separable problems / 1" is real but is a second, independent problem I did not measure, so it should not sit inside the measured block. Your proposed direction — count matches, refuse and ask, reuse the rejection path, add a deterministicORDER BY, and leave prefix length as a separate product decision — I agree with as written.Findings
apps/management/lib/bot/nodes/execute-purchase.ts:74: should: neither bot node's use ofdescribeBotRejectionis asserted, so the operator-facing half of the whole-record-rejection decision can be deleted silently. Measured on both nodes — remove theconst rejected = describeBotRejection(…)call and itsif (rejected) returnguard, leaving the existing fallback, andtscexits 0 with 318 of 318 passing.execute-stock-adjust.ts:29is the same. This is the fourth appearance of the shape the last four rounds have been about: the helper is pinned, the caller is not. Both nodes are plain functions overBotState, so mockingdband asserting the reply contains the field label andTidak ada data yang tersimpan sama sekali.closes it in two tests.apps/management/lib/mcp/tools/orders.ts:113: should: theINVALID_INPUTbranch typechecks away and no test notices —tscexit 0, 318 passing. Dropping it hands an MCP client{ ok: false, code: "INVALID_INPUT", items: undefined }instead of the message, which is a bounds rejection arriving at an LLM agent with the reason removed. The other two transports are covered (purchases.test.tsfor the route, and the orders route is type-guarded), so this is the one uncovered transport rather than a class.apps/management/lib/stock-movements/service.ts:33: nit:lockInventoryRows' docblock says it takes "every row lock a multi-line write will need", and that is not true fornewItemlines — their ids do not exist when it runs, andcreatePurchasefilters them out viaid > 0. Harmless, per the reasoning above, but the sentence overstates the guarantee, and this PR's history is mostly comments that claimed slightly more than the code did. "Every lock for a line that names an existing row" is both accurate and still the point.apps/management/lib/purchases/service.ts:44: nit: two purchases that each create the same two new item names in opposite line order deadlock on the uncommitted unique-index entries forlower(btrim(item_name))— measured,40P01. Pre-existing and not yours: I diffed thenewItembranch ofupsertPurchaseItemagainstorigin/mainand it is identical apart fromError→ServiceRejection, andlockInventoryRowsnever runs for an all-nullline set. Narrow reachability (two receipts introducing the same two new parts concurrently, in different order) and it belongs with #81's neighbourhood rather than here. Flagged so it is not rediscovered as a regression later.apps/management/lib/stock-movements/service.ts:69: nit: a transaction that callsadjustStockmore than once takes one lock per call in call order, so it deadlocks againstcreatePurchase's ascending pre-lock — measured,40P01on 4 of 4 rounds with a caller adjustingBthenA. No such caller exists today: all threeadjustStockcall sites (mcp/tools/stock-movements.ts:43,stock-movements/actions.ts:31,bot/nodes/execute-stock-adjust.ts:20) do exactly one call per transaction, so this is latent rather than live and I am not filing it as a defect. It becomes live the first time anyone adds a bulk stock-take, whichKoreksi Auditmakes a plausible feature. One line in the docblock saying multi-row callers must go throughlockInventoryRowsfirst would cost nothing and is the cheapest place to stop it.apps/management/lib/bot/nodes/execute-order.ts:49: nit: carried and still open —Order #${orderId}interpolates the{ok, id}object, so the reply readsOrder #[object Object].apps/management/lib/orders/service.ts:201: nit: carried and still open —OrderStockError's message is"Insufficient lot stock"but it is thrown for a physical-count shortage too.Why this is an approve. The blocker chain is done: bounds at the service with every caller enumerated, the resulting-stock invariant closed on all four write sites, the negative-stock race locked, the deadlock that lock bought removed and measured clean across nine pairings,
201-on-rejection replaced by a declared 400, and rejections that reach the operator in a production build rather than only innext dev— I verified that last one in a realnext startbuild rather than trusting it. The remaining findings are two testability gaps and four notes on comments and pre-existing edges; none changes what the code does.classify-intent.ts'sOutputParserExceptionnote is the one carried item I would still like closed, but it is message quality on a path where the service bound holds and nothing is written, and it has been correctly scoped as such for four rounds.Everything above ran locally; nothing checked this branch automatically (#73). Not filed, per scope: #77, #70/#76, #73, #72, #81.
Database disclosure. Started Postgres on the existing datadir (it was not running) and stopped it; nothing bound on 5432 at finish, and I started no HTTP server this round. Committed writes only to fixtures I created —
__m5_*inventory rows and the purchases, orders, lots and movements the deadlock matrix made through them — all deleted by the matrix itself, which I then verified rather than assumed. Baseline rows 4 and 5 were never used as fixtures and never written:4|65000|85000|12,5|25000|40000|8,weight_gramsNULL, both published. Finalmd5(string_agg(inventory))=187c8b21e6425ee4489fa22612c1d447, byte-identical to the value captured before rounds 1, 2, 3, 4 and 5; ledger tables all 0; migrations 12/12; sessions 9 with no new users;op44,op59andsvc71left alone. The user'sstash@{0}is untouched andgit status --porcelainon the default checkout shows onlyM .omp/mcp.json.Review 69: approve-with-nits, 0 blockers, 2 shoulds, 5 nits. The 40P01 matrix is clean — nine pairings, opposite line order throughout, round 4's 6-of-6 deadlock now 0-of-4 on the same fixture, including the three cases I asked to be attacked and a fully-disjoint swap. `lockInventoryRows`' plan was checked rather than its clause: `LockRows` above `Sort (Sort Key: id)` in both the narrow and wider `IN` forms. **The fourth instance of this PR's recurring defect, closed as a class.** Three links were genuinely undefended — `tsc` 0 AND the whole suite green with the code deleted: execute-stock-adjust.ts describeBotRejection call + guard removable execute-purchase.ts same mcp/tools/orders.ts the INVALID_INPUT arm of the response ternary The two bot nodes are the worst of them: `rejection.test.ts` pins the helper, so the phrasing was defended while nothing proved either node calls it — the operator-facing half of the whole-record-rejection decision was silently removable. That decision is the reason a refused receipt says "Tidak ada data yang tersimpan sama sekali" instead of a generic failure, and the purchase node is where it matters most, since its numbers come from OCR on a photograph and land on `inventory.buy_price`. `node-wiring.test.ts` and `orders-wiring.test.ts` assert the nodes and the tool deliver the rejection, each with a negative control so they cannot pass on code that answers the refusal text for everything — including a real outage. No MCP harness was needed: `registerOrderTools` only calls `server.tool(name, description, schema, handler)`, so a fake that captures handlers by name reaches the real one. **That new test immediately found a defect nobody had looked for.** MCP `create_order` called `requestStoreDeploy("order created via MCP")` BEFORE checking `result.ok`, so a rejected order queued a storefront rebuild — and the comment above it read "Stock moved", which was false on exactly the path that reached it without moving any. Now inside the success branch. Audited the siblings rather than assuming: `purchases.ts` (x3) and `stock-movements.ts` all `await db.transaction(...)` on services that THROW, so their deploy is unreachable on a rejection. `create_order` was the only returning-result path, and so the only one that could fall through. **Deliberately NOT tested, on the reviewer's ruling:** `orders/actions.ts` bypassing `runAction` and the orders route dropping its `INVALID_INPUT` throw both fail `tsc` with exit 2 (TS2339 on `ActionResult<CreateOrderResult>`). Widening that type made the compiler the guard, which is stronger than a test because it cannot be skipped. **Two comment nits, both cases of this PR's history claiming slightly more than the code does.** `lockInventoryRows` said it takes "every row lock a multi-line write will need", which is untrue for `newItem` lines — they have no id until the loop inserts them and are filtered by `id > 0`. It now says "rows that ALREADY EXIST" and records why that is safe rather than a hole, with the reviewer's measurement: a null line interleaved with an existing item in the opposite order returns `ServiceRejection | ok`, because the pre-lock runs before the loop wherever the null line sits, and a row this transaction just inserted cannot be lock-contended since it is not yet visible. The docblock also now states the caller obligation the reviewer measured but correctly did not file as a defect: a transaction taking more than one stock lock must come through `lockInventoryRows` first, because two `lockStockForChange` calls in caller order deadlock against this ascending pre-lock (4 of 4, adjusting B then A). Not live — all three call sites do exactly one per transaction — and recorded so a future bulk stock-take does not make it live silently. **Three issues filed rather than widening this PR:** - **#81 corrected.** The measured block read as though both rows existed; in the reviewer's run only the 10W-40 row did, and the incoming name resolved onto it. That matters because it means NO ambiguity is required — one matching row is enough — and the new product is never created, so one row accumulates two products' stock. The multi-match non-determinism is a second, unmeasured problem and no longer sits inside the measured evidence. - **#82**: the four dashboard rejection-rendering branches can each be deleted with the suite green. Closing it needs a DOM environment and a component library, neither of which exists here — real scope, and the wrong thing to bolt onto a bounds-and-locking PR. The manual evidence across five rounds of production builds is recorded in the issue. - **#83**: pre-existing on `origin/main` — two concurrent purchases creating the same new item names in opposite order deadlock on the uncommitted unique-index entries for `lower(btrim(item_name))`. `lockInventoryRows` cannot help, since an all-`newItem` line set issues no pre-lock at all. Round-5 battery, every mutation diffed: 25 stock-adjust node drops the guard caught (was green) 26 purchase node drops the guard caught (was green) 27 mcp drops the INVALID_INPUT arm caught (was green) 28 mcp deploys before checking ok caught (new defect) 324 tests, up from 318. tsc exit 0. biome over the PR's 27 changed files: 3 warnings, all pre-existing on `origin/main` and none on a line this PR touches.VERDICT: approve-with-nits
Scoped to the delta
ce27c0b..e27bce4, as asked. Nothing blocks merge. All three undefended links from review 69 are closed with working negative controls, the deploy defect the new test found is real and correctly fixed, and the sibling audit's conclusion holds — though its stated reason covers fewer sites than it claims. Everything left is comment precision and two carried cosmetics.vitest run→ 16 files, 324 passed;tsc --noEmit→ exit 0; biome over the branch's changed files → the same 3 pre-existing warnings, none on a touched line. (The count is 29 files now, not 27 — the two new test files. Immaterial, but this PR's theme has been numbers that drift.)The three links are closed, and the controls actually control
And the negative controls are not decorative. Making a genuine outage answer the refusal text — a plain
Errorfalling into the "nothing was saved" branch — fails four tests named for exactly that property: "leaves a genuine fault alone rather than dressing it up as a bad number", "still reports a genuine fault as itself". MakingdescribeBotRejectionreturn a fixed string for everything fails eleven. So these cannot pass on a node that refuses indiscriminately, which was the risk worth guarding. Re-verified the earlier set too: droppingcreatePurchase's pre-lock,lockInventoryRows'orderBy, orcreateOrder's inventory lock still fail 1, 4 and 3 tests.The deploy defect, and the audit
The defect is real and the fix is right. Deploying before
if (result.ok)meant a rejected MCP order queued a storefront rebuild under a comment reading "Stock moved" — false on the only path that reached it. Finding it by writing a test for something else is the argument for the tests.The audit's conclusion holds:
create_orderwas the only site that deployed on a rejection, and no sibling has the same bug. I checked all fourteenrequestStoreDeploycall sites, not the five named:orders/actions.ts:36,stock-movements/actions.ts:36,purchases/actions.ts:32,44— theif (!result.ok) returnprecedes the deploy. Safe.purchases/actions.ts:51,mcp/tools/purchases.ts:66,164,189,mcp/tools/stock-movements.ts:46— services throw. Safe, and this is the group the audit describes.mcp/tools/inventory.ts:122,259,306— these do not throw. They return{ isError: true, … }for a name conflict, a missing slug, an invalid id, "no fields to update" and "used in an order" — seven returning rejections across three tools. Every one returns before the write, so it returns before the deploy. Safe, but not for the reason given.inventory/actions.ts:180—if (Object.keys(updates).length === 0) return;is an early return too, also ahead of the deploy.:99and:202throw.mcp/tools/inventory.ts:364is the manualdeploy_storetool and is unconditional by design.So: eight non-throwing paths the audit did not enumerate, none of them a bug. Worth knowing because "the siblings throw" is a property that can stop being true, whereas "the return precedes the write" is the invariant that actually holds — and
create_orderbroke precisely because it returned after.The corrected comments
lockInventoryRows' new text is accurate against my measurements on every clause I can check: theid > 0filter, the pre-lock running before the loop regardless of where thenullline sits, theServiceRejection | okresult, the 4-of-4 multi-call measurement, and the three call sites doing one adjust per transaction. The caller obligation is the right thing to record and the right place to record it.One clause does overclaim, and it is the one you invited me to look for — see the nit below. Short version: "cannot be lock-contended" is true of row locks and false if read as "cannot deadlock", and #83 is the counterexample.
Your two questions
classify-intent'sOutputParserException: file it, do not do it here. Three reasons. It is message quality on a path where the service bound holds and nothing is written — I measured that in round 1 and it has not changed. The fix requires a decision about how to unwrap a langchainOutputParserException— re-parsellmOutputwith the schema, or match the class by name — which is a question about langchain error handling generally, not about #71's bounds. Andvision-llm.tscarries the same wrap, correctly hedged, so a proper fix touches both nodes and wants its own change rather than a sixth round on a PR that is now about locking. The only thing I would insist on is that the issue records thatclassify-intent.ts's comment currently claims behaviour the code does not produce, since that comment ships either way.#83's reason is not quite right, and I have commented the correction rather than editing it. "An all-
newItemset issues no pre-lock at all" is true but implies a set that does issue one is protected. Measured:The real reason is that a row lock cannot order contention on an index entry for a row that does not exist yet. The shared-existing-item case is clean only incidentally, because the pre-lock on
Aserialises the transactions before either reaches its inserts. That changes what a fix would be: ordering thenewItemnames before inserting would work; extendinglockInventoryRowscannot, because there is nothing to lock. #81 and #82 I have no corrections on — #81 now reflects my measurement, and #82 records the production-build evidence accurately, which matters because it is the only evidence that exists for those four links.Findings
apps/management/lib/stock-movements/service.ts:38: nit: "a row this transaction just inserted is not visible to any other transaction, so it cannot be lock-contended" is true of row locks and invites the wrong conclusion. Two concurrent purchases creating the same two new names in opposite order do deadlock, on the uncommitted unique-index entries — that is #83, and I measured it again this round including a mixed set where a pre-lock is issued and does not help. Since this paragraph exists to explain why excludingnewItemlines is safe, a reader will take it as "these lines cannot deadlock", which is the one thing it must not mean. One clause — "row locks; index-entry contention between two not-yet-existing rows is #83" — makes it exact, and this is a comment about locking in the file that owns locking.apps/management/lib/mcp/tools/orders.ts:99: nit: the sibling audit recorded in the fix comment generalises from the wrong property.mcp/tools/inventory.tshas seven returning rejections across three tools andinventory/actions.ts:174has an early return, so eight of the fourteen deploy sites are not "services that throw" — they are safe because the return precedes the write. All correct today; the note just would not catch the nextcreate_order, which was a returning path that returned too late. Naming the invariant as "every rejection returns before the write, and the deploy is after the write" would.apps/management/lib/bot/nodes/classify-intent.ts:401: nit, downgraded from should and to be filed separately per the above:withStructuredOutputwraps the schema failure inOutputParserException, sodescribeBotRejectionreturnsnulland the node returns the bareclassifiedIntent: "UNKNOWN"this comment says it replaces. Nothing is written and the service bound holds; the comment is the wrong part and ships either way.apps/management/lib/bot/nodes/execute-order.ts:49: nit: carried and still open —Order #${orderId}interpolates the{ok, id}object, so the reply readsOrder #[object Object].apps/management/lib/orders/service.ts:201: nit: carried and still open —OrderStockError's message is"Insufficient lot stock"but it is thrown for a physical-count shortage too.Nothing blocks merge. The five nits are two comment-precision fixes, one issue to file, and two one-line cosmetics that have been open since round 1 and change no behaviour. Merging deploys the management container; I have exercised the paths this PR changes against real Postgres and real production builds across six rounds, and the last four rounds found defects only in the guards the previous round added — which has now stopped: this round's mutations all fail as they should, and the one defect found was found by the PR's own new test rather than by me.
Everything above ran locally; nothing checked this branch automatically (#73). Not filed, per scope: #77, #70/#76, #73, #72, #81, #82, #83.
Database disclosure. Started Postgres on the existing datadir (it was not running) and stopped it; nothing bound on 5432 at finish, and I started no HTTP server this round. Committed writes only to
__x83_*fixtures for the mixed-set deadlock probe and the purchases they created, all deleted and then verified rather than assumed. Baseline rows 4 and 5 were never used as fixtures and never written. Finalmd5(string_agg(inventory))=187c8b21e6425ee4489fa22612c1d447, byte-identical to the value captured before rounds 1 through 6; ledger tables all 0; migrations 12/12; sessions 9 with no new users;op44,op59andsvc71left alone. The user'sstash@{0}is untouched andgit status --porcelainon the default checkout shows onlyM .omp/mcp.json.