diff --git a/CLAUDE.md b/CLAUDE.md index 7bc9d75..06d1419 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -658,14 +658,33 @@ and the cross-repo contract are in smarthome `plans/intake-router-slice-plan.md` the email/order bridge books orders on its own schedule; `findDuplicateBatch` ignores *unresolved* batches by design, so two pending cards for one order would otherwise each book it. -- **One `expense_metadata` row, agreed on by three queries.** A transaction can - own a row *and* be the `matched_transaction_id` of another; `matchTransaction` - returns the row it resolved through and `replaceLineItems` writes to exactly - that one. All three (match, write, `GET /api/transactions/[id]/order`) order - by `(transaction_id = $1) DESC **NULLS LAST**, id` — the `NULLS LAST` is - load-bearing, because Postgres orders `DESC` as `NULLS FIRST` and the row that - merely *points* at the transaction has a NULL `transaction_id`, so the clause - meant to prefer the owned row preferred the other one. +- **One `expense_metadata` row, agreed on by every query that resolves one.** A + transaction can own a row *and* be the `matched_transaction_id` of another; + `matchTransaction` returns the row it resolved through and `replaceLineItems` + writes to exactly that one. Every reader orders by + `(transaction_id = $1) DESC **NULLS LAST**, id` — the match, the write, the + order-detail GET, `getTransactions`' provenance LATERAL, `merchantForTransaction` + and the Slack details modal. The `NULLS LAST` is load-bearing: Postgres orders + `DESC` as `NULLS FIRST` and the row that merely *points* at the transaction has + a NULL `transaction_id`, so the clause meant to prefer the owned row preferred + the other one. Measured 2026-08-21: 667 rows, 147 pointing-only, **zero** + transactions resolving to more than one row — the ordering changes nothing + today and exists so the readers still agree when multiplicity appears. +- **The create is guarded by a transaction-scoped advisory lock on the + PURCHASE**, not the batch. `ON CONFLICT (source, source_ref)` handles a double + tap on one card; two *different* batches for one purchase have different + `source_ref`s and both clear their match and duplicate checks before either + inserts. The lock has to live inside `withTransaction` (`src/lib/db.ts`): + `queryRaw` is `$queryRawUnsafe` on a **pooled** client, so every call is its + own implicit transaction and `pg_advisory_xact_lock` taken in a statement of + its own is released before the next statement runs. The loser reports + `duplicate` pointing at the winner's transaction — the same answer the + sequential path gives. +- **`replaceLineItems` snapshots the undo baseline BEFORE it writes, on both + paths**, and treats an UPDATE that touched no row as a failure rather than a + success. The metadata INSERT is `ON CONFLICT (transaction_id) DO UPDATE` and + carries the batch's **currency** — a hard-coded `'AUD'` made `OrderDetails` + print `$` on a USD order, because it prefers `expense_metadata.currency`. - **`intake_batches.previous_line_items` is the undo baseline, written once.** Returning whatever the write just read makes a pantry retry get its own write back as the "previous" state, so undo restores the thing it undid. Recorded diff --git a/src/__tests__/integration/helpers.ts b/src/__tests__/integration/helpers.ts index f57cac9..99d6e97 100644 --- a/src/__tests__/integration/helpers.ts +++ b/src/__tests__/integration/helpers.ts @@ -23,6 +23,25 @@ export function mockDbWithPool(p: Pool) { const result = await p.query(sql, params); return result.rows[0] ?? null; }, + // Mirrors withTransaction: one connection, BEGIN/COMMIT around the callback, + // so pg_advisory_xact_lock actually holds across the statements inside it. + withTransaction: async (fn: (q: unknown) => Promise) => { + const client = await p.connect(); + try { + await client.query("BEGIN"); + const result = await fn({ + queryRaw: async (sql: string, params: unknown[] = []) => (await client.query(sql, params)).rows, + queryRow: async (sql: string, params: unknown[] = []) => (await client.query(sql, params)).rows[0] ?? null, + }); + await client.query("COMMIT"); + return result; + } catch (e) { + await client.query("ROLLBACK"); + throw e; + } finally { + client.release(); + } + }, prisma: p, })); } diff --git a/src/__tests__/integration/intake.test.ts b/src/__tests__/integration/intake.test.ts index 50dc68d..dc2e95a 100644 --- a/src/__tests__/integration/intake.test.ts +++ b/src/__tests__/integration/intake.test.ts @@ -250,6 +250,35 @@ describe("PUT /api/transactions/[id]/line-items (C2)", () => { expect(second.previous_line_items).toEqual(items); }); + it("a pinned row that no longer exists falls back to the live one, rather than reporting a phantom write", async () => { + const txn = await seedOrder({ merchant: "Burgertory (Werribee)", date: "2026-08-19", amount: 22.4 }); + const live = (await pool.query(`SELECT id FROM expense_metadata WHERE transaction_id = $1`, [txn])).rows[0].id as number; + const { replaceLineItems } = await import("../../lib/line-items"); + const r = await replaceLineItems(txn, items, { source: "intake", metadataId: live + 999 }); + expect(r.metadata_id).toBe(live); + expect(await lineItemsOf(txn)).toHaveLength(2); + }); + + it("two writes racing to create the metadata row both resolve to the same one", async () => { + const txn = await seedOrder({ merchant: "Coles", date: "2026-08-18", amount: 85.74, withMeta: false }); + const both = await Promise.all([ + put(txn, { line_items: items, source: "intake", replace: true }), + put(txn, { line_items: items, source: "intake", replace: true }), + ]); + expect(both.map((r) => r.status)).toEqual([200, 200]); + expect((await pool.query(`SELECT count(*)::int AS n FROM expense_metadata WHERE transaction_id = $1`, [txn])).rows[0].n).toBe(1); + }); + + it("the create path records an undo baseline too, before it writes", async () => { + const txn = await seedOrder({ merchant: "Coles", date: "2026-08-18", amount: 85.74, withMeta: false }); + const id = uuid(); + await pool.query(`INSERT INTO intake_batches (id, source, file_sha256, status) VALUES ($1::uuid, 'other', '{}', 'received')`, [id]); + await put(txn, { line_items: items, source: "intake", batch_id: id, currency: "AUD", replace: true }); + // [] rather than NULL: there was nothing there, and "nothing" is the state + // undo has to be able to restore. + expect((await pool.query(`SELECT previous_line_items FROM intake_batches WHERE id = $1`, [id])).rows[0].previous_line_items).toEqual([]); + }); + it("the panel reads the row the write landed on, whichever order the rows were inserted in", async () => { const t = await seedOrder({ merchant: "UBER *EATS", date: "2026-08-19", amount: 22.4, withMeta: false }); // Inserted first: the delivery receipt pointing at the statement line. @@ -670,6 +699,42 @@ describe("Slack taps (C5)", () => { const t = (await pool.query(`SELECT amount::text, amount_aud, foreign_currency_amount::text, foreign_currency_code FROM transactions`)).rows[0]; expect(t).toEqual({ amount: "22.40", amount_aud: null, foreign_currency_amount: "22.40", foreign_currency_code: "USD" }); expect(lastSlackUpdate().text).toMatch(/USD 22\.40/); + // OrderDetails prefers expense_metadata.currency, so a hard-coded 'AUD' + // here printed "$" on a USD order's receipt panel. + expect((await pool.query(`SELECT currency FROM expense_metadata`)).rows[0].currency).toBe("USD"); + }); + + it("the hold applies to the Create tap's re-match, not only to the upload path", async () => { + modelReply = { ...burgertory, subtotal: null, items: burgertory.items.map((i) => ({ ...i, unit_price: null, line_total: null })) }; + const id = uuid(); + const body = await (await intakePOST(multipart({ source: "uber_eats", batch_id: id }, [pdf]))).json(); + expect(body.status).toBe("candidates"); + + // Booked WITH its items between the card and the tap. The re-match walked + // past the guard routeFood applies, so the same upload destroyed the same + // receipt depending on which button reached it. + const txn = await seedOrder({ merchant: "Burgertory (Werribee)", date: "2026-08-19", amount: 22.4 }); + const already = [{ qty: 1, description: "the itemised receipt already there", amount: 22.4 }]; + await pool.query(`UPDATE expense_metadata SET line_items = $2::jsonb WHERE transaction_id = $1`, [txn, JSON.stringify(already)]); + + await slackPOST(slackPress("intake_create", { batch_id: id })); + expect(await lineItemsOf(txn)).toEqual(already); + expect(await batch(id)).toMatchObject({ status: "held" }); + expect((await pool.query(`SELECT count(*)::int AS n FROM transactions`)).rows[0].n).toBe(1); + }); + + it("two DIFFERENT batches for one purchase, tapped together, book it once", async () => { + const a = uuid(); + const b = uuid(); + await intakePOST(multipart({ source: "uber_eats", batch_id: a }, [pdf])); + await intakePOST(multipart({ source: "uber_eats", batch_id: b }, [{ name: "shot.png", type: "image/png", bytes: Buffer.from("shot") }])); + // Their source_refs differ, so ON CONFLICT cannot save this; both taps + // clear their match and duplicate checks before either inserts. + await Promise.all([slackPOST(slackPress("intake_create", { batch_id: a })), slackPOST(slackPress("intake_create", { batch_id: b }))]); + const rows = (await pool.query(`SELECT id FROM transactions`)).rows; + expect(rows).toHaveLength(1); + expect((await batch(a)).transaction_id).toBe(rows[0].id); + expect((await batch(b)).transaction_id).toBe(rows[0].id); }); it("unknown shared_by falls back to the default owner — never a NULL owner", async () => { diff --git a/src/__tests__/unit/intake-rules.test.ts b/src/__tests__/unit/intake-rules.test.ts index efd41f1..ee9cd27 100644 --- a/src/__tests__/unit/intake-rules.test.ts +++ b/src/__tests__/unit/intake-rules.test.ts @@ -234,7 +234,7 @@ describe("cards carry the C5 action ids and JSON values", () => { expect(ids).toContain('"action_id":"intake_meal"'); }); - it("mrkdwn escapes the names it did not write; the plain_text button keeps them raw", () => { + it("every value the card did not write is escaped, plain_text included", () => { const hostile = { ...c, merchant: "Ben & Jerry's " }; const { text, blocks } = candidatesCard(hostile, [ { transaction_id: 42, description: "SQ *BEN & JERRY ", transaction_date: "2026-08-17", amount: "19.90" }, @@ -242,24 +242,38 @@ describe("cards carry the C5 action ids and JSON values", () => { const sections = JSON.stringify(blocks.filter((b) => (b as Block).type === "section")); expect(sections).toContain("Ben & Jerry's <fake>"); expect(sections).toContain("SQ *BEN & JERRY <AU>"); - expect(sections).not.toContain(""); expect(text).toContain("Ben & Jerry's <fake>"); - // The Create button's label is plain_text — Slack shows entities literally - // there, so escaping it would print "Ben & Jerry's" on the button. - const actions = JSON.stringify(blocks.filter((b) => (b as Block).type === "actions")); - expect(actions).toContain("Ben & Jerry's "); + // plain_text disables formatting, not entity decoding — &, < and > are + // control characters everywhere in a Slack payload. + const whole = JSON.stringify(blocks); + expect(whole).not.toContain(""); + expect(whole).not.toContain(""); }); - it("a foreign receipt is not printed as dollars", () => { + it("the currency is model output too, so it is escaped like any other", () => { + // parseExtraction accepts any string for `currency`; unescaped, `<@U123>` + // would be a live mention on every card the batch produces. + const { text } = enrichedCard({ ...c, currency: "<@U123>", transactionId: 7 }); + expect(text).toContain("<@U123> 22.40"); + expect(text).not.toContain("<@U123>"); + }); + + it("a foreign receipt is not printed as dollars — header and candidate rows alike", () => { expect(enrichedCard({ ...c, currency: "USD", transactionId: 7 }).text).toBe( "Added 2 items to your USD 22.40 Uber Eats (Burgertory (Werribee)) order ✓" ); expect(enrichedCard({ ...c, currency: "AUD", transactionId: 7 }).text).toContain("$22.40"); expect(enrichedCard({ ...c, transactionId: 7 }).text).toContain("$22.40"); + // A candidate row is a ledger transaction and carries its own currency; it + // used to print "$" beneath a header reading "USD 22.40". + const { blocks } = candidatesCard({ ...c, currency: "USD" }, [ + { transaction_id: 42, description: "Order - Burgertory", transaction_date: "2026-08-17", amount: "19.90", currency: "USD" }, + ]); + expect(JSON.stringify(blocks)).toContain("USD 19.90"); }); }); -describe("liteLLMExtractor — only a finished reply is accepted", () => { +describe("liteLLMExtractor — a finish_reason that is present must be `stop`", () => { const good = { merchant: "Burgertory (Werribee)", order_date: "2026-08-19", total: 22.4, subtotal: 28, currency: "AUD", items: [], adjustments: [] }; const extractor = (finish_reason?: string) => liteLLMExtractor({ @@ -271,12 +285,13 @@ describe("liteLLMExtractor — only a finished reply is accepted", () => { })) as unknown as typeof fetch, }); - it("accepts `stop`, and a reply that reports no reason at all", async () => { + it("accepts `stop`, and — on purpose — a reply that reports no reason at all", async () => { expect((await extractor("stop")([{ kind: "text", text: "x", name: "x" }])).total).toBe(22.4); expect((await extractor()([{ kind: "text", text: "x", name: "x" }])).total).toBe(22.4); }); it("rejects any other reason, naming it — the strict schema only held for a reply that finished", async () => { + // An absent reason is NOT rejected: see the comment in intake-extract.ts. await expect(extractor("length")([{ kind: "text", text: "x", name: "x" }])).rejects.toThrow(ExtractionError); await expect(extractor("length")([{ kind: "text", text: "x", name: "x" }])).rejects.toThrow(/too long/); await expect(extractor("content_filter")([{ kind: "text", text: "x", name: "x" }])).rejects.toThrow(/content_filter/);