From e81d07975ad8b8c64cf7b417f3d4b5310b031a67 Mon Sep 17 00:00:00 2001 From: marcuspaico Date: Mon, 17 Aug 2026 16:17:03 -0700 Subject: [PATCH] fix(labs): strict numeric parsing, atomic confirm guard, test integrity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - normalize.ts num(): parseFloat truncated at the first comma, so "1,200" silently became 1 (1000x error) and "5,5" became 5. Now strictly matches either US thousands-grouping or a plain number spanning the whole string; anything else (incl. ambiguous "5,5") returns null instead of a wrong value. - labs.ts confirm handler: the pending-status check ran before the request body was read, so two concurrent confirms could both pass it and double-insert. Added a guarded UPDATE ... WHERE status = 'pending' as the first statement inside the existing synchronous transaction; zero rows affected throws and the route returns 409, with the fast-path check kept for the common case. - Added missing `await` on two rejects.toThrow assertions (llm.test.ts, extract.test.ts) that were previously resolving before the assertion settled. - Bumped the 11th-failed-login rate-limit test to a 30s timeout — 10 sequential argon2id verifies can exceed bun:test's 5s default under load. --- server/src/lib/normalize.ts | 22 +++++++++++++-- server/src/routes/labs.ts | 46 +++++++++++++++++++++++--------- server/test/auth.test.ts | 2 +- server/test/extract.test.ts | 2 +- server/test/labs-confirm.test.ts | 7 +++++ server/test/llm.test.ts | 2 +- server/test/normalize.test.ts | 17 ++++++++++++ 7 files changed, 80 insertions(+), 18 deletions(-) diff --git a/server/src/lib/normalize.ts b/server/src/lib/normalize.ts index fc7c8db..eb12484 100644 --- a/server/src/lib/normalize.ts +++ b/server/src/lib/normalize.ts @@ -24,9 +24,27 @@ export interface NormMarker { canonicalUnit: string | null; } +const THOUSANDS_GROUPED = /^\d{1,3}(,\d{3})+(\.\d+)?$/; +const PLAIN_NUMBER = /^-?\d+(\.\d+)?$/; + +// Strict numeric parsing: parseFloat alone stops at the first non-numeric +// character, so "1,200" silently became 1 (a 1000x error) and "5,5" (a +// European decimal) silently became 5. Instead: strip comparators/whitespace, +// then only accept (a) US thousands-grouping, comma-stripped, or (b) a plain +// number that spans the ENTIRE remaining string. Anything else — including +// ambiguous "5,5" — returns null so no canonical value is computed rather +// than a silently wrong one. const num = (v: string): number | null => { - const n = parseFloat(v.replace(/[<>≤≥]/g, "").trim()); - return Number.isFinite(n) ? n : null; + const stripped = v.replace(/[<>≤≥\s]/g, ""); + if (THOUSANDS_GROUPED.test(stripped)) { + const n = parseFloat(stripped.replace(/,/g, "")); + return Number.isFinite(n) ? n : null; + } + if (PLAIN_NUMBER.test(stripped)) { + const n = parseFloat(stripped); + return Number.isFinite(n) ? n : null; + } + return null; }; export function normalizeMarker(raw: RawMarker): NormMarker { diff --git a/server/src/routes/labs.ts b/server/src/routes/labs.ts index 2228626..e552a94 100644 --- a/server/src/routes/labs.ts +++ b/server/src/routes/labs.ts @@ -1,4 +1,5 @@ -import { desc, eq } from "drizzle-orm"; +import type { Changes } from "bun:sqlite"; +import { and, desc, eq } from "drizzle-orm"; import { Hono } from "hono"; import { randomUUID } from "node:crypto"; import { existsSync, mkdirSync } from "node:fs"; @@ -88,20 +89,39 @@ export function labsRoutes(deps: LabsDeps) { panel: m.panel, name: m.name, value: m.value, unit: m.unit, referenceRange: m.referenceRange, flagged: m.flagged, })); - deps.db.transaction((tx) => { - tx.insert(labDraws).values({ - id: drawId, collectedAt: body.data.collectedDate, labName: body.data.labName, - draftId: row.id, createdAt: Date.now(), - }).run(); - for (const n of norm) { - tx.insert(biomarkers).values({ - drawId, panel: n.panel, name: n.name, marker: n.marker, analyteKey: n.analyteKey, - value: n.value, valueNum: n.valueNum, unit: n.unit, referenceRange: n.referenceRange, - flagged: n.flagged ? 1 : 0, valueCanonical: n.valueCanonical, canonicalUnit: n.canonicalUnit, + try { + deps.db.transaction((tx) => { + // Guarded status transition (pending -> confirmed) inside the same + // synchronous transaction as the inserts below, so two concurrent + // confirms of the same draft can't both pass the earlier status + // check (before the request body was even read) and double-insert. + // Only the request that actually flips the row gets to write rows. + // drizzle-orm's bun-sqlite types pin TRunResult to `void`, but at + // runtime bun:sqlite's Statement.run() actually returns a + // `{ changes, lastInsertRowid }` Changes object — confirmed in + // node_modules/bun-types/sqlite.d.ts. Cast to the real runtime type. + const updated = tx.update(labDrafts).set({ status: "confirmed" }) + .where(and(eq(labDrafts.id, row.id), eq(labDrafts.status, "pending"))).run() as unknown as Changes; + if (updated.changes === 0) throw new Error("draft_not_pending"); + + tx.insert(labDraws).values({ + id: drawId, collectedAt: body.data.collectedDate, labName: body.data.labName, + draftId: row.id, createdAt: Date.now(), }).run(); + for (const n of norm) { + tx.insert(biomarkers).values({ + drawId, panel: n.panel, name: n.name, marker: n.marker, analyteKey: n.analyteKey, + value: n.value, valueNum: n.valueNum, unit: n.unit, referenceRange: n.referenceRange, + flagged: n.flagged ? 1 : 0, valueCanonical: n.valueCanonical, canonicalUnit: n.canonicalUnit, + }).run(); + } + }); + } catch (e) { + if (e instanceof Error && e.message === "draft_not_pending") { + return c.json({ error: "draft is not pending" }, 409); } - tx.update(labDrafts).set({ status: "confirmed" }).where(eq(labDrafts.id, row.id)).run(); - }); + throw e; + } return c.json({ drawId }, 201); }); diff --git a/server/test/auth.test.ts b/server/test/auth.test.ts index d20436c..2f4cf2c 100644 --- a/server/test/auth.test.ts +++ b/server/test/auth.test.ts @@ -54,7 +54,7 @@ describe("auth", () => { // The global window applies to everyone, including a request with the correct password. const blocked = await app.request("/api/login", json({ password: "hunter2hunter2" })); expect(blocked.status).toBe(429); - }); + }, 30000); // 10 sequential argon2id verifies can exceed the 5s default timeout test("unknown /api/* returns 404 when authed, 401 when unauthed", async () => { const app = makeApp(); diff --git a/server/test/extract.test.ts b/server/test/extract.test.ts index a80faa6..3118e57 100644 --- a/server/test/extract.test.ts +++ b/server/test/extract.test.ts @@ -41,6 +41,6 @@ describe("extractFromText", () => { test("malformed LLM output → llm_error, not a crash", async () => { const mock = (async () => new Response(JSON.stringify({ choices: [{ message: { content: '{"markers": "not an array"}' } }] }), { status: 200 })) as unknown as typeof fetch; const dir = mkdtempSync(join(tmpdir(), "helios-")); - expect(extractFromText({ db: openDb(dir), key: loadOrCreateKey(dir), fetchImpl: mock }, "x")).rejects.toThrow(/llm_error/); + await expect(extractFromText({ db: openDb(dir), key: loadOrCreateKey(dir), fetchImpl: mock }, "x")).rejects.toThrow(/llm_error/); }); }); diff --git a/server/test/labs-confirm.test.ts b/server/test/labs-confirm.test.ts index d19846e..a126b54 100644 --- a/server/test/labs-confirm.test.ts +++ b/server/test/labs-confirm.test.ts @@ -64,6 +64,13 @@ describe("draft review", () => { body: JSON.stringify({ collectedDate: "2026-01-15", labName: null, markers: [{ panel: null, name: "X", value: "1", unit: null, referenceRange: null, flagged: false }] }), }); expect(again.status).toBe(409); + + // Second (rejected) confirm must not have inserted a second draw or any + // extra biomarker rows — exactly one draw, one set of biomarkers. + const drawsAfter = await db.select().from(labDraws); + expect(drawsAfter).toHaveLength(1); + const rowsAfter = await db.select().from(biomarkers); + expect(rowsAfter).toHaveLength(2); }); test("discard marks draft discarded", async () => { diff --git a/server/test/llm.test.ts b/server/test/llm.test.ts index b090d80..eeea086 100644 --- a/server/test/llm.test.ts +++ b/server/test/llm.test.ts @@ -46,6 +46,6 @@ describe("chatJSON", () => { test("non-2xx throws llm_error", async () => { const mock = (async () => new Response("nope", { status: 401 })) as unknown as typeof fetch; - expect(chatJSON(deps(mock), { system: "s", user: "u" })).rejects.toThrow(/llm_error/); + await expect(chatJSON(deps(mock), { system: "s", user: "u" })).rejects.toThrow(/llm_error/); }); }); diff --git a/server/test/normalize.test.ts b/server/test/normalize.test.ts index ebf338c..fe13987 100644 --- a/server/test/normalize.test.ts +++ b/server/test/normalize.test.ts @@ -61,4 +61,21 @@ describe("normalizeMarker", () => { const n = normalizeMarker({ name: "DHEA-S", value: "250", unit: "ug/dL" }); expect(n.valueCanonical).toBeCloseTo(6.78, 1); // 250 * 0.02713 }); + test("thousands-grouped value parses as 1200, not 1 (parseFloat truncation bug)", () => { + const n = normalizeMarker({ name: "Vitamin B12", value: "1,200", unit: "pg/mL" }); + expect(n.valueNum).toBe(1200); + expect(n.analyteKey).toBe("vitamin_b12"); + expect(n.valueCanonical).toBeCloseTo(885.35, 1); + expect(n.canonicalUnit).toBe("pmol/L"); + }); + test("ambiguous European-decimal-looking value is left unmapped, not silently wrong", () => { + const n = normalizeMarker({ name: "Glucose", value: "5,5", unit: "mg/dL" }); + expect(n.valueNum).toBeNull(); + expect(n.valueCanonical).toBeNull(); + expect(n.value).toBe("5,5"); // raw value preserved + }); + test("comparator value still parses after strict-parsing rewrite", () => { + const n = normalizeMarker({ name: "hs-CRP", value: "<0.3", unit: "mg/L" }); + expect(n.valueNum).toBeCloseTo(0.3); + }); });