From 669f9119b9c30a416c8f4be1551f469edc3dd087 Mon Sep 17 00:00:00 2001 From: siddharthd Date: Sat, 22 Aug 2026 15:57:31 +1000 Subject: [PATCH] enrichment readers: resolve one metadata row, deterministically (board #253) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four readers of expense_metadata that only the 1:1 UNIQUE constraints keep correct, found while designing wallet-first ingestion. Each fails silently rather than erroring, so none of them would announce itself. merchantVerdict (order-reviews.ts) was a genuine fan-out: a plain JOIN on transaction_id OR matched_transaction_id, no per-transaction cap, feeding `for (const r of rows) counts[r.rating] += 1`. One review joined to N metadata rows is counted N times, inflating the loved/never tallies that drive the pre-order warning — and its LIMIT 50 applies to the fanned-out set, so duplicates would also push older reviews out of the window. Now a LATERAL, so the merchant matched is the merchant the UI shows. nudgeState (slack/interactive) had no LIMIT and no ORDER BY, feeding queryRow, which returns rows[0]. Postgres guarantees no ordering, so the merchant and currency on every Slack card refresh could flip at random. It was the only "resolve both links" site missing the convention its siblings all carry. getSharedTransactions had LATERAL LIMIT 1 with no ORDER BY — bounded, but arbitrary, and free to disagree with the main transaction list about the same transaction. transaction_search (migration 0035): NOT the fan-out I first claimed. It joined only on em.transaction_id, which is UNIQUE, so it could not multiply rows and sums over it were never inflated. The real defect is narrower and worse: it OMITTED matched_transaction_id, so every card-settled order was invisible to it — 148 transactions (Glovo Milan, Uber Eats, all source='order-bridge') reporting NULL payment_method / order_reference / line_items while the data sat in the table. This view is the agent read surface (Hermes' MCP selects those columns from it) and home-dashboard reads it via dash_ro, so the omission was estate-wide. Broadening the join is what introduces fan-out risk — transaction_id and matched_transaction_id are two SEPARATE unique indexes, so two different rows can legally point at one transaction — hence LATERAL rather than a second OR. Verified before applying: 4760 rows unchanged, debit sum unchanged at 1514412.89, only the 148 rows differ. Grants re-granted and re-verified, since CREATE OR REPLACE VIEW drops them. Also fixes an integration assertion 258fe3b left stale: it changed the needs-header card text and updated the unit tests but not intake.test.ts. The new assertion checks both halves of needsHeaderCard — that it names what is actually missing, and that a non-in-store source gets the share-more-of-the-screen advice. npm run test:all: 182 unit + 276 integration, all passing. --- .../migration.sql | 73 +++++++++++++++++++ src/__tests__/integration/intake.test.ts | 8 +- src/app/api/slack/interactive/route.ts | 17 ++++- src/lib/order-reviews.ts | 21 +++++- src/lib/queries.ts | 5 ++ 5 files changed, 116 insertions(+), 8 deletions(-) create mode 100644 prisma/migrations/0035_transaction_search_matched_txn/migration.sql diff --git a/prisma/migrations/0035_transaction_search_matched_txn/migration.sql b/prisma/migrations/0035_transaction_search_matched_txn/migration.sql new file mode 100644 index 0000000..34b9279 --- /dev/null +++ b/prisma/migrations/0035_transaction_search_matched_txn/migration.sql @@ -0,0 +1,73 @@ +-- transaction_search: resolve expense_metadata by BOTH links, without fanning out. +-- +-- THE DEFECT (measured 2026-08-22): the view joined only on +-- `em.transaction_id = t.id`, so it silently omitted every card-settled order, +-- which carries only `matched_transaction_id`. 148 transactions are affected -- +-- Glovo orders in Milan, Uber Eats, all `source='order-bridge'` -- and for each +-- of them the view reports NULL payment_method / order_reference / line_items +-- while the data sits in the table. +-- +-- That is not cosmetic. This view is the agent read surface: Hermes' +-- postgres-personal MCP selects payment_method and order_reference from it +-- (personal-agent-gateway/mcp-servers/postgres-personal/queries.py), and +-- home-dashboard reads it via dash_ro. CLAUDE.md states the rule the view broke: +-- "That lookup must match transaction_id OR matched_transaction_id: a +-- card-settled order has only the latter." +-- +-- NOT a fan-out fix, contrary to a first reading. The original join CANNOT fan +-- out: expense_metadata_transaction_id_key is UNIQUE on transaction_id, so it +-- matched at most one row by construction. Sums taken over this view were never +-- inflated. +-- +-- But broadening the join is exactly what INTRODUCES that risk, which is why +-- LATERAL is used rather than a second OR'd condition. transaction_id and +-- matched_transaction_id are enforced by two SEPARATE unique indexes +-- (expense_metadata_transaction_id_key, uq_expense_matched_txn), so one +-- transaction can legally be pointed at by two DIFFERENT rows -- one via each +-- link. Zero such transactions exist today; the LATERAL means that stays a +-- non-event rather than a silent duplicate in the agent's spending totals. +-- +-- Tie-break matches every other reader (getTransactions, replaceLineItems, +-- matchTransaction, /api/transactions/[id]/order), so the row this view resolves +-- is the row the UI shows. +-- +-- Verified before applying: row count unchanged at 4760 (= count(transactions)), +-- sum(amount) over debits unchanged at 1514412.89 -- amount comes from +-- transactions, never from the metadata row -- and the only difference is the +-- 148 rows that gain the metadata they always should have had. +-- +-- CREATE OR REPLACE VIEW DROPS GRANTS. dash_ro (home-dashboard) and agent_ro +-- (Hermes) both read this view and losing either breaks a surface with no error +-- at replace time, so both are re-granted below. + +CREATE OR REPLACE VIEW transaction_search AS + SELECT t.id, + t.transaction_date, + t.description, + COALESCE(t.merchant_normalized, t.merchant_name) AS merchant, + t.category, + t.transaction_type, + COALESCE(t.amount_aud, t.amount) AS amount, + t.foreign_currency_amount, + t.foreign_currency_code, + t.location, + s.bank_name, + s.card_name, + s.account_number, + s.statement_type, + s.billing_end_date, + em.payment_method, + em.order_reference, + em.line_items + FROM transactions t + LEFT JOIN statements s ON s.id = t.statement_id + LEFT JOIN LATERAL ( + SELECT em2.payment_method, em2.order_reference, em2.line_items + FROM expense_metadata em2 + WHERE em2.transaction_id = t.id OR em2.matched_transaction_id = t.id + ORDER BY (em2.transaction_id = t.id) DESC NULLS LAST, em2.id + LIMIT 1 + ) em ON true; + +GRANT SELECT ON transaction_search TO dash_ro; +GRANT SELECT ON transaction_search TO agent_ro; diff --git a/src/__tests__/integration/intake.test.ts b/src/__tests__/integration/intake.test.ts index 53f36ab..cc61a76 100644 --- a/src/__tests__/integration/intake.test.ts +++ b/src/__tests__/integration/intake.test.ts @@ -541,7 +541,13 @@ describe("POST /api/intake — food (enrich vs candidates)", () => { const id = uuid(); const body = await (await intakePOST(multipart({ source: "uber_eats", batch_id: id }, [pdf]))).json(); expect(body.status).toBe("needs_header"); - expect(body.text).toMatch(/share the top of the screen/); + // Asserts BOTH halves of needsHeaderCard(): that it names what is actually + // missing (total, not a blanket "total or date"), and that a uber_eats + // source gets the share-more-of-the-screen advice rather than the in-store + // Share-button advice. 258fe3b changed this text and updated the unit tests + // but not this one. + expect(body.text).toMatch(/I can see items but no total/); + expect(body.text).toMatch(/share the part of the screen that shows it/); expect(await lineItemsOf(txn)).toEqual([]); expect((await batch(id)).status).toBe("needs_header"); }); diff --git a/src/app/api/slack/interactive/route.ts b/src/app/api/slack/interactive/route.ts index 2286bbe..8fc2829 100644 --- a/src/app/api/slack/interactive/route.ts +++ b/src/app/api/slack/interactive/route.ts @@ -387,12 +387,21 @@ async function nudgeState(transactionId: number) { currency: string | null; amount: string; }>( - `SELECT em.merchant_normalized AS merchant, - COALESCE(em.currency, 'AUD') AS currency, + // LATERAL + the shared tie-break, not a bare join: `queryRow` returns + // rows[0] and Postgres guarantees no ordering, so a second metadata row on + // one transaction would flip the merchant and currency on every card + // refresh at random. Every sibling reader already resolves this way. + `SELECT order_ctx.merchant_normalized AS merchant, + COALESCE(order_ctx.currency, 'AUD') AS currency, t.amount FROM transactions t - LEFT JOIN expense_metadata em - ON em.transaction_id = t.id OR em.matched_transaction_id = t.id + LEFT JOIN LATERAL ( + SELECT em.merchant_normalized, em.currency + FROM expense_metadata em + WHERE em.transaction_id = t.id OR em.matched_transaction_id = t.id + ORDER BY (em.transaction_id = t.id) DESC NULLS LAST, em.id + LIMIT 1 + ) order_ctx ON true WHERE t.id = $1`, [transactionId] ); diff --git a/src/lib/order-reviews.ts b/src/lib/order-reviews.ts index 46739e7..9d6942a 100644 --- a/src/lib/order-reviews.ts +++ b/src/lib/order-reviews.ts @@ -167,9 +167,24 @@ export async function merchantVerdict( FROM order_reviews r JOIN transactions t ON t.id = r.transaction_id JOIN participants p ON p.id = r.participant_id - JOIN expense_metadata em - ON em.transaction_id = r.transaction_id - OR em.matched_transaction_id = r.transaction_id + -- (No backticks in here: this SQL lives in a TS template literal and a + -- backtick ends the string -- TS1005 on a line that looks like a comment.) + -- LATERAL, not a plain JOIN. A plain join on + -- transaction_id OR matched_transaction_id fans out: one review + -- joined to N metadata rows appears N times in the result, and the loops + -- below count every appearance -- inflating the loved/never tallies + -- that drive the pre-order warning. LIMIT 50 applies to the fanned-out + -- set too, so duplicates would also push older reviews out of the + -- window. Same tie-break as every other reader, so the merchant matched + -- here is the merchant the UI shows. + JOIN LATERAL ( + SELECT em2.merchant_normalized + FROM expense_metadata em2 + WHERE em2.transaction_id = r.transaction_id + OR em2.matched_transaction_id = r.transaction_id + ORDER BY (em2.transaction_id = r.transaction_id) DESC NULLS LAST, em2.id + LIMIT 1 + ) em ON true -- Case-folded: the platforms capitalise the same restaurant differently -- ("TEG Kebabs & Biryani" on Uber Eats, "TEG KEBABS & BIRYANI" on -- DoorDash). An exact match split one restaurant's history in two, so a diff --git a/src/lib/queries.ts b/src/lib/queries.ts index b18b0c2..f171e99 100644 --- a/src/lib/queries.ts +++ b/src/lib/queries.ts @@ -990,6 +990,11 @@ export async function getSharedTransactions(ownerId: number, tagIds?: number[], SELECT em.platform FROM expense_metadata em WHERE em.transaction_id = t.id OR em.matched_transaction_id = t.id + -- Same tie-break as getTransactions / replaceLineItems / matchTransaction, + -- so every reader resolves the row a write lands on. Without the ORDER BY + -- this LIMIT 1 is bounded but arbitrary: it would disagree with the main + -- transaction list for the same transaction the day multiplicity appears. + ORDER BY (em.transaction_id = t.id) DESC NULLS LAST, em.id LIMIT 1 ) order_ctx ON true LEFT JOIN transactions src ON src.reconciled_with_id = t.id AND src.statement_id IS NULL