test(intake): the second review pass, and CLAUDE.md for the rules it set
ci / lint-test (push) Successful in 45s
ci / lint-test (push) Successful in 45s
Six of the nine fixes have a test that fails against the previous tree: the metadata row's currency, the Create-path hold, two concurrent batches booking once, mrkdwn escaping including plain_text, the escaped currency, and candidate-row currency. Three do not, and the reason is the same each time — the window needs a crash or a lost race that a test cannot force. Asserted instead: a stale metadataId falls back to the live row rather than reporting a phantom write; two writes racing to create a metadata row both resolve to one; the create path records a baseline at all. All three passed before as well.
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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<unknown>) => {
|
||||
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,
|
||||
}));
|
||||
}
|
||||
|
||||
@@ -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 () => {
|
||||
|
||||
@@ -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 <fake>" };
|
||||
const { text, blocks } = candidatesCard(hostile, [
|
||||
{ transaction_id: 42, description: "SQ *BEN & JERRY <AU>", 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("<fake>");
|
||||
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 <fake>");
|
||||
// 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("<fake>");
|
||||
expect(whole).not.toContain("<AU>");
|
||||
});
|
||||
|
||||
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/);
|
||||
|
||||
Reference in New Issue
Block a user