enrichment readers: resolve one metadata row, deterministically (board #253)
ci / lint-test (push) Successful in 53s
ci / lint-test (push) Successful in 53s
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.
This commit is contained in:
@@ -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;
|
||||||
@@ -541,7 +541,13 @@ describe("POST /api/intake — food (enrich vs candidates)", () => {
|
|||||||
const id = uuid();
|
const id = uuid();
|
||||||
const body = await (await intakePOST(multipart({ source: "uber_eats", batch_id: id }, [pdf]))).json();
|
const body = await (await intakePOST(multipart({ source: "uber_eats", batch_id: id }, [pdf]))).json();
|
||||||
expect(body.status).toBe("needs_header");
|
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 lineItemsOf(txn)).toEqual([]);
|
||||||
expect((await batch(id)).status).toBe("needs_header");
|
expect((await batch(id)).status).toBe("needs_header");
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -387,12 +387,21 @@ async function nudgeState(transactionId: number) {
|
|||||||
currency: string | null;
|
currency: string | null;
|
||||||
amount: string;
|
amount: string;
|
||||||
}>(
|
}>(
|
||||||
`SELECT em.merchant_normalized AS merchant,
|
// LATERAL + the shared tie-break, not a bare join: `queryRow` returns
|
||||||
COALESCE(em.currency, 'AUD') AS currency,
|
// 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
|
t.amount
|
||||||
FROM transactions t
|
FROM transactions t
|
||||||
LEFT JOIN expense_metadata em
|
LEFT JOIN LATERAL (
|
||||||
ON em.transaction_id = t.id OR em.matched_transaction_id = t.id
|
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`,
|
WHERE t.id = $1`,
|
||||||
[transactionId]
|
[transactionId]
|
||||||
);
|
);
|
||||||
|
|||||||
@@ -167,9 +167,24 @@ export async function merchantVerdict(
|
|||||||
FROM order_reviews r
|
FROM order_reviews r
|
||||||
JOIN transactions t ON t.id = r.transaction_id
|
JOIN transactions t ON t.id = r.transaction_id
|
||||||
JOIN participants p ON p.id = r.participant_id
|
JOIN participants p ON p.id = r.participant_id
|
||||||
JOIN expense_metadata em
|
-- (No backticks in here: this SQL lives in a TS template literal and a
|
||||||
ON em.transaction_id = r.transaction_id
|
-- backtick ends the string -- TS1005 on a line that looks like a comment.)
|
||||||
OR em.matched_transaction_id = r.transaction_id
|
-- 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
|
-- Case-folded: the platforms capitalise the same restaurant differently
|
||||||
-- ("TEG Kebabs & Biryani" on Uber Eats, "TEG KEBABS & BIRYANI" on
|
-- ("TEG Kebabs & Biryani" on Uber Eats, "TEG KEBABS & BIRYANI" on
|
||||||
-- DoorDash). An exact match split one restaurant's history in two, so a
|
-- DoorDash). An exact match split one restaurant's history in two, so a
|
||||||
|
|||||||
@@ -990,6 +990,11 @@ export async function getSharedTransactions(ownerId: number, tagIds?: number[],
|
|||||||
SELECT em.platform
|
SELECT em.platform
|
||||||
FROM expense_metadata em
|
FROM expense_metadata em
|
||||||
WHERE em.transaction_id = t.id OR em.matched_transaction_id = t.id
|
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
|
LIMIT 1
|
||||||
) order_ctx ON true
|
) order_ctx ON true
|
||||||
LEFT JOIN transactions src ON src.reconciled_with_id = t.id AND src.statement_id IS NULL
|
LEFT JOIN transactions src ON src.reconciled_with_id = t.id AND src.statement_id IS NULL
|
||||||
|
|||||||
Reference in New Issue
Block a user