From 7a755f64069945c47b138d7a6d39d3d3f0f1a71f Mon Sep 17 00:00:00 2001 From: Lucas Winther Date: Sat, 5 Sep 2026 22:58:20 +0200 Subject: [PATCH] =?UTF-8?q?fix(validation):=20review=20round=201=20?= =?UTF-8?q?=E2=80=94=20close=20the=20store's=20source=20hole,=20fix=20orde?= =?UTF-8?q?ring=20and=20messages?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I1: a calendar charge's MISSING occurrenceDate no longer reports the literal word "undefined" (String(undefined) printed it, and that text appears nowhere in the file someone is searching). Missing and malformed are split into two messages; the missing one names the cost, matching the anchored side's message. I2: addPurchase/updatePurchase accepted an arbitrary `source` unchecked — only confirmOccurrence built a coherent one. A later task hand-builds a source and passes it straight to addPurchase, so this closes it at the store layer now rather than leaving that task to carry the validation. Both enforce the same pairing the importer does: a calendar cost's source requires occurrenceDate, an anchored cost's source forbids it, an unknown cost throws. updatePurchase checks the resolved pair (incoming source if given, else the stored one), the same style already used for anchoring. M1: the "no recurring costs" fold test added last round was a duplicate of an existing one and couldn't fail independently. Replaced with a test that actually discriminates which charge an anchored cost's walk steps from — a 10-day cycle matching the gap between three charges, where anchoring off the earliest instead of the latest changes whether anything is still expected in the same month. Verified by making that swap in occurrences.ts, watching it fail, and reverting. M2: updateRecurringCost now refuses a charged cost's anchoring change before checking startDate granularity, so a doomed request fails on the real reason first rather than making the owner fix an unrelated date problem and resubmit into the same wall. Also closed two pre-existing asymmetries with json.ts: requireRecurrence now rejects an unknown recurrence kind, and both addRecurringCost and updateRecurringCost reject an unknown anchoring — both previously relied on TypeScript's unions alone, which a hand-edited object or an `as` cast does not go through. --- src/domain/fold.test.ts | 39 ++++++--- src/export/json.test.ts | 24 ++++++ src/export/json.ts | 16 +++- src/store/actions.test.ts | 161 ++++++++++++++++++++++++++++++++++++++ src/store/actions.ts | 108 ++++++++++++++++++++++--- 5 files changed, 322 insertions(+), 26 deletions(-) diff --git a/src/domain/fold.test.ts b/src/domain/fold.test.ts index 1426888..63e94a7 100644 --- a/src/domain/fold.test.ts +++ b/src/domain/fold.test.ts @@ -346,16 +346,33 @@ describe("the anchored (lastCharge) track", () => { expect(january.expected).toBe(0); // nothing left uncharged in January itself }); - test("with no recurring costs, projected still equals remaining", () => { - // The anchored model adds a second walk (`anchoredOccurrences`), but a - // dataset that never defines a recurring cost must never see it: both - // walks are keyed off `dataset.recurring`, which is empty here. - const data = dataset([post("food", 500)], [spend("a", "food", 300, "2026-01-05")]); - const fold = foldBalances(data, "2026-03"); - for (const month of ["2026-01", "2026-02", "2026-03"]) { - const figures = figuresFor(fold, "food", month); - expect(figures.expected).toBe(0); - expect(figures.projected).toBe(figures.remaining); - } + // REVIEW ROUND 1 / M1: the three-charges test above pins that January's + // `expected` is 0, but that assertion holds no matter WHICH charge the walk + // anchors from — with a 30-day cycle, even the earliest of the three + // (2026-01-05) steps to 2026-02-04, past January, so `charges[0]` in place + // of `charges[charges.length - 1]` in `anchoredOccurrences` would still + // pass it. This test uses a cycle short enough (10 days, matching the gap + // between each charge) that anchoring on the wrong end changes the answer: + // stepping from the EARLIEST charge (2026-01-05) lands on 2026-01-15 and + // then 2026-01-25 — both still inside January, both unconfirmed by + // definition (an anchored occurrence is never matched against an existing + // Purchase) — which would double-count 600 as still owed in a month that + // in fact has nothing left uncharged. Anchoring on the LATEST charge + // (2026-01-25) instead steps to 2026-02-04, outside January, leaving + // nothing expected there. Verified by swapping `charges[length - 1]` for + // `charges[0]` in `anchoredOccurrences` and watching this fail before + // reverting. + test("expected in January depends on anchoring off the LATEST charge, not the earliest", () => { + const data = datasetWithAnchoredPlan(); + data.recurring[0]!.recurrence = { kind: "everyNDays", n: 10 }; + data.purchases.push( + { id: "p1", date: "2026-01-05", description: "Phone plan", total: { amount: 300, currency: "DKK" }, splitMode: "percent", splits: [{ postId: "phone", value: 100, absorbsRemainder: true }], schedule: null, source: { recurringId: "plan" } }, + { id: "p2", date: "2026-01-15", description: "Phone plan", total: { amount: 300, currency: "DKK" }, splitMode: "percent", splits: [{ postId: "phone", value: 100, absorbsRemainder: true }], schedule: null, source: { recurringId: "plan" } }, + { id: "p3", date: "2026-01-25", description: "Phone plan", total: { amount: 300, currency: "DKK" }, splitMode: "percent", splits: [{ postId: "phone", value: 100, absorbsRemainder: true }], schedule: null, source: { recurringId: "plan" } }, + ); + + const january = figuresFor(foldBalances(data, "2026-01"), "phone", "2026-01"); + expect(january.charges).toBe(900); // 3 x 300, all real money already moved + expect(january.expected).toBe(0); // correct anchor steps to 2026-02-04, outside January }); }); diff --git a/src/export/json.test.ts b/src/export/json.test.ts index ce223e3..eaef723 100644 --- a/src/export/json.test.ts +++ b/src/export/json.test.ts @@ -606,6 +606,30 @@ describe("the occurrenceDate / anchoring pairing", () => { test("an anchored charge without occurrenceDate is accepted", () => { expect(() => parseDatasetJson(withCostAndCharge("lastCharge", { recurringId: "r1" }))).not.toThrow(); }); + + // REVIEW ROUND 1 / I1: a calendar charge with a genuinely MISSING + // occurrenceDate used to be reported with `String(undefined)`, which prints + // the literal word "undefined" — so someone restoring a backup would + // search their file for that text and find nothing, since the file has no + // such string anywhere. Missing and malformed are different problems, and + // the missing case now names the cost instead. + test("a calendar charge missing occurrenceDate names the cost, not the word 'undefined'", () => { + let message = ""; + try { + parseDatasetJson(withCostAndCharge("calendar", { recurringId: "r1" })); + } catch (error) { + message = (error as Error).message; + } + expect(message).toMatch(/Bill/); // the cost's name, from withCostAndCharge's fixture + expect(message).not.toMatch(/undefined/); + }); + + // The malformed case (present but not a real date) keeps its old shape. + test("a calendar charge with a malformed occurrenceDate still names the bad value", () => { + expect(() => + parseDatasetJson(withCostAndCharge("calendar", { recurringId: "r1", occurrenceDate: "nope" })), + ).toThrow(/invalid occurrence date "nope"/); + }); }); describe("startDate granularity under the anchored model", () => { diff --git a/src/export/json.ts b/src/export/json.ts index 0fed439..ff6537a 100644 --- a/src/export/json.ts +++ b/src/export/json.ts @@ -450,9 +450,21 @@ export function parseDatasetJson(text: string): Dataset { ); } } else { - if (!PURCHASE_DATE.test(source.occurrenceDate as string)) { + // Split from the "malformed" case below: `String(undefined)` prints + // the literal word "undefined", which sends someone restoring a + // backup searching their file for text that appears nowhere in it. + // Missing and malformed are different problems and need different + // messages, and this one names the cost the way the anchored-side + // message above already does. + if (source.occurrenceDate === undefined) { throw new ImportValidationError( - `${label} has an invalid occurrence date "${String(source.occurrenceDate)}"`, + `${label} has no occurrence date, but "${cost.name}" is calendar-anchored and a charge ` + + `against it settles one slot`, + ); + } + if (!PURCHASE_DATE.test(source.occurrenceDate)) { + throw new ImportValidationError( + `${label} has an invalid occurrence date "${source.occurrenceDate}"`, ); } } diff --git a/src/store/actions.test.ts b/src/store/actions.test.ts index 596fbeb..3836940 100644 --- a/src/store/actions.test.ts +++ b/src/store/actions.test.ts @@ -947,3 +947,164 @@ describe("confirmOccurrence by anchoring", () => { expect(p.date).toBe("2026-09-01"); }); }); + +// REVIEW ROUND 1 / I2: `addPurchase`/`updatePurchase` used to accept an +// arbitrary `source` unchecked — only `confirmOccurrence` built a coherent +// one. A later task adds a control that hand-builds a `source` and passes it +// straight to `addPurchase`, so this closes it here rather than leaving that +// task to carry the validation. Mirrors the pairing `json.ts` enforces at +// import: a source naming a calendar cost requires `occurrenceDate`; one +// naming an anchored cost forbids it; one naming a nonexistent cost throws. +describe("addPurchase/updatePurchase enforce the source/occurrenceDate pairing", () => { + function seeded() { + const d = createSeedDataset("2026-09"); + const post = d.posts[0]!; + const calendarCost = actions.addRecurringCost(d, { + name: "Rent", archived: false, + amount: { amount: 8000, currency: "DKK" }, + startDate: "2026-09", + recurrence: { kind: "everyNMonths", n: 1 }, + anchoring: "calendar", splitMode: "percent", + splits: [{ postId: post.id, value: 100, absorbsRemainder: true }], + }); + const anchoredCost = actions.addRecurringCost(d, { + name: "Phone", archived: false, + amount: { amount: 300, currency: "DKK" }, + startDate: "2026-09-01", + recurrence: { kind: "everyNMonths", n: 1 }, + anchoring: "lastCharge", splitMode: "percent", + splits: [{ postId: post.id, value: 100, absorbsRemainder: true }], + }); + return { d, post, calendarCost, anchoredCost }; + } + + function purchaseInput( + postId: string, + source?: { recurringId: string; occurrenceDate?: string }, + ) { + return { + date: "2026-09-15", + description: "Test", + total: { amount: 100, currency: "DKK" }, + splitMode: "percent" as const, + splits: [{ postId, value: 100, absorbsRemainder: true }], + schedule: null, + source, + }; + } + + test("addPurchase refuses a source naming an unknown recurring cost", () => { + const { d, post } = seeded(); + expect(() => actions.addPurchase(d, purchaseInput(post.id, { recurringId: "ghost" }))) + .toThrow(/Unknown recurring cost/); + }); + + test("addPurchase requires occurrenceDate when source names a calendar cost", () => { + const { d, post, calendarCost } = seeded(); + expect(() => actions.addPurchase(d, purchaseInput(post.id, { recurringId: calendarCost.id }))) + .toThrow(/occurrence date/i); + }); + + test("addPurchase accepts a calendar source WITH occurrenceDate", () => { + const { d, post, calendarCost } = seeded(); + expect(() => + actions.addPurchase(d, purchaseInput(post.id, { recurringId: calendarCost.id, occurrenceDate: "2026-09" })), + ).not.toThrow(); + }); + + test("addPurchase forbids occurrenceDate when source names an anchored cost", () => { + const { d, post, anchoredCost } = seeded(); + expect(() => + actions.addPurchase( + d, + purchaseInput(post.id, { recurringId: anchoredCost.id, occurrenceDate: "2026-09-01" }), + ), + ).toThrow(/occurrence date/i); + }); + + test("addPurchase accepts an anchored source with NO occurrenceDate", () => { + const { d, post, anchoredCost } = seeded(); + expect(() => + actions.addPurchase(d, purchaseInput(post.id, { recurringId: anchoredCost.id })), + ).not.toThrow(); + }); + + test("addPurchase with no source at all is unaffected", () => { + const { d, post } = seeded(); + expect(() => actions.addPurchase(d, purchaseInput(post.id, undefined))).not.toThrow(); + }); + + test("updatePurchase checks the RESOLVED pair against a source it newly sets", () => { + const { d, post, anchoredCost } = seeded(); + const purchase = actions.addPurchase(d, purchaseInput(post.id, undefined)); + expect(() => + actions.updatePurchase(d, purchase.id, { + source: { recurringId: anchoredCost.id, occurrenceDate: "2026-09-01" }, + }), + ).toThrow(/occurrence date/i); + }); + + test("updatePurchase re-validates the STORED source when an unrelated field changes", () => { + const { d, post, calendarCost } = seeded(); + const purchase = actions.addPurchase( + d, + purchaseInput(post.id, { recurringId: calendarCost.id, occurrenceDate: "2026-09" }), + ); + expect(() => actions.updatePurchase(d, purchase.id, { description: "Renamed" })).not.toThrow(); + expect(d.purchases[0]!.description).toBe("Renamed"); + }); +}); + +// REVIEW ROUND 1: pre-existing asymmetries with json.ts, confirmed clean +// against the baseline — neither `requireRecurrence` nor `addRecurringCost`/ +// `updateRecurringCost` validated `recurrence.kind`/`anchoring` against their +// known sets; both relied on TypeScript's unions alone, which do not survive +// a hand-edited object or an `as` cast reaching this file directly. +describe("runtime guards beyond what TypeScript's unions promise", () => { + function draft(): Dataset { + const data = createSeedDataset("2026-01"); + data.posts[0]!.id = "housing"; + return data; + } + + const billInput = { + name: "Bill", archived: false, + amount: { amount: 100, currency: "DKK" }, startDate: "2026-01-01", + recurrence: { kind: "everyNMonths" as const, n: 1 }, + anchoring: "calendar" as const, splitMode: "percent" as const, + splits: [{ postId: "housing", value: 100, absorbsRemainder: true }], + }; + + test("addRecurringCost refuses an unknown recurrence kind bypassing the type checker", () => { + const data = draft(); + expect(() => + actions.addRecurringCost(data, { + ...billInput, + recurrence: { kind: "everyFullMoon", n: 1 } as any, + }), + ).toThrow(/recurrence/i); + }); + + test("addRecurringCost refuses an unknown anchoring bypassing the type checker", () => { + const data = draft(); + expect(() => + actions.addRecurringCost(data, { ...billInput, anchoring: "whenever" as any }), + ).toThrow(/anchoring/i); + }); + + test("updateRecurringCost refuses an unknown anchoring the same way", () => { + const data = draft(); + const cost = actions.addRecurringCost(data, billInput); + expect(() => + actions.updateRecurringCost(data, cost.id, { anchoring: "whenever" as any }), + ).toThrow(/anchoring/i); + }); + + test("updateRecurringCost refuses an unknown recurrence kind the same way", () => { + const data = draft(); + const cost = actions.addRecurringCost(data, billInput); + expect(() => + actions.updateRecurringCost(data, cost.id, { recurrence: { kind: "everyFullMoon", n: 1 } as any }), + ).toThrow(/recurrence/i); + }); +}); diff --git a/src/store/actions.ts b/src/store/actions.ts index 27414ab..6926dda 100755 --- a/src/store/actions.ts +++ b/src/store/actions.ts @@ -204,6 +204,7 @@ export function addPurchase( draft: Dataset, purchase: Omit, ): Purchase { + requireSourcePairing(draft, purchase.source); const created: Purchase = { ...purchase, id: newId(), @@ -228,6 +229,12 @@ export function updatePurchase( ): void { const purchase = requirePurchase(draft, purchaseId); const resolved: Partial> = { ...changes }; + // The RESOLVED pair, same `??` style `updateRecurringCost` uses for + // anchoring/recurrence above: the incoming `source` if this update sets + // one, else the one already stored — so touching an unrelated field still + // re-validates whatever source is already there, and a `source` this + // update DOES set is checked against the cost it now names. + requireSourcePairing(draft, changes.source ?? purchase.source); if (changes.total) { resolved.total = roundMoneyValue(draft, changes.total); } @@ -375,6 +382,66 @@ function requireRecurringCost(draft: Dataset, id: RecurringCostId): RecurringCos return cost; } +/** + * REVIEW ROUND 1 / I2: `addPurchase`/`updatePurchase` used to accept an + * arbitrary `source` unchecked — only `confirmOccurrence` built a coherent + * one. That let the store write a dataset that would fail its own + * re-import, which is exactly the drift the boundary-agreement requirement + * exists to close. A later task adds a control that hand-builds a `source` + * and passes it straight to `addPurchase`, so this has to be closed here, at + * the store layer, rather than becoming a validation that control's own task + * has to carry. + * + * Mirrors the pairing `json.ts` enforces at the import boundary: a `source` + * naming a calendar cost REQUIRES an `occurrenceDate` (a charge against a + * fixed grid settles one slot); a `source` naming a `lastCharge` cost + * FORBIDS one (an anchored cost has no grid, so a slot id would describe a + * schedule this app will not compute). `undefined` (no source at all) is + * always fine and skips this entirely. + */ +function requireSourcePairing(draft: Dataset, source: Purchase["source"]): void { + if (source === undefined) return; + const cost = requireRecurringCost(draft, source.recurringId); + if (cost.anchoring === "lastCharge") { + if (source.occurrenceDate !== undefined) { + throw new Error( + `"${cost.name}" is anchored to its last charge and has no slots to settle, so a purchase ` + + `referencing it must not carry an occurrence date`, + ); + } + } else if (source.occurrenceDate === undefined) { + throw new Error( + `"${cost.name}" is calendar-anchored, so a purchase referencing it must carry the occurrence ` + + `date (slot) it settles`, + ); + } +} + +/** + * REVIEW ROUND 1: pre-existing asymmetry with `json.ts`'s `RECURRENCE_KINDS` + * check — this file relied on the `Recurrence` union alone, which only binds + * a caller that goes through the type checker. A hand-edited object or an + * `as` cast reaches this function directly (this is the store's write path, + * not the JSON one), so the three known kinds are checked here too now. + */ +const RECURRENCE_KINDS = new Set(["everyNMonths", "everyNDays", "everyNWeeks"]); + +/** Mirrors `json.ts`'s `ANCHORINGS` check for the same reason: the `Anchoring` + * union only binds a typed caller. */ +const ANCHORINGS = new Set(["calendar", "lastCharge"]); + +/** + * Guards against a value TypeScript's `Anchoring` union would otherwise + * guarantee — but only for a caller the type checker actually saw. A + * hand-edited object or an `as` cast reaches this function directly. + */ +function requireAnchoring(anchoring: Anchoring): Anchoring { + if (!ANCHORINGS.has(anchoring)) { + throw new Error(`Anchoring must be "calendar" or "lastCharge", not "${anchoring}"`); + } + return anchoring; +} + /** * The projection walk in `domain/occurrences.ts` terminates only if every step * strictly advances, and `n` is what guarantees that. The JSON importer checks @@ -382,6 +449,9 @@ function requireRecurringCost(draft: Dataset, id: RecurringCostId): RecurringCos * fold can hang. */ function requireRecurrence(recurrence: Recurrence): Recurrence { + if (!RECURRENCE_KINDS.has(recurrence.kind)) { + throw new Error(`A recurrence must be "everyNMonths", "everyNDays", or "everyNWeeks", not "${recurrence.kind}"`); + } if (!Number.isInteger(recurrence.n)) { throw new Error(`A recurrence interval must be a whole number, not ${recurrence.n}`); } @@ -442,13 +512,15 @@ export function addRecurringCost( draft: Dataset, cost: Omit, ): RecurringCost { + const anchoring = requireAnchoring(cost.anchoring); const recurrence = requireRecurrence(cost.recurrence); - requireStartDateGranularity(recurrence, cost.startDate, cost.anchoring); + requireStartDateGranularity(recurrence, cost.startDate, anchoring); const created: RecurringCost = { ...cost, id: newId(), order: draft.recurring.length, recurrence, + anchoring, amount: roundMoneyValue(draft, cost.amount), splits: roundSplits(draft, cost.splits, cost.splitMode, cost.amount.currency), }; @@ -473,20 +545,16 @@ export function updateRecurringCost( const cost = requireRecurringCost(draft, id); const resolved: Partial> = { ...changes }; - if (changes.recurrence) { - resolved.recurrence = requireRecurrence(changes.recurrence); + if (changes.anchoring) { + requireAnchoring(changes.anchoring); } - // Runs on every update, not just one that touches recurrence or startDate: - // either field alone can break the pairing (a kind flip against an - // untouched month-only startDate is exactly how this bug was reached), and - // an anchoring flip against an untouched month-only startDate is the same - // shape of bug one level up. - requireStartDateGranularity( - resolved.recurrence ?? cost.recurrence, - resolved.startDate ?? cost.startDate, - changes.anchoring ?? cost.anchoring, - ); + // REVIEW ROUND 1 / M2: checked BEFORE requireStartDateGranularity below. + // A charged cost's anchoring change is refused outright, no matter what + // else is wrong with the request — checking the date pairing first meant + // flipping a charged month-only calendar cost to lastCharge reported the + // date problem, the owner "fixed" the date and resubmitted, and only then + // learned the change was never allowed at all. if (changes.anchoring && changes.anchoring !== cost.anchoring) { // Switching calendar -> lastCharge could strip the now-forbidden slot ids // mechanically, but the reverse cannot: there is no honest way to invent @@ -502,6 +570,20 @@ export function updateRecurringCost( } } + if (changes.recurrence) { + resolved.recurrence = requireRecurrence(changes.recurrence); + } + // Runs on every update, not just one that touches recurrence or startDate: + // either field alone can break the pairing (a kind flip against an + // untouched month-only startDate is exactly how this bug was reached), and + // an anchoring flip against an untouched month-only startDate is the same + // shape of bug one level up. + requireStartDateGranularity( + resolved.recurrence ?? cost.recurrence, + resolved.startDate ?? cost.startDate, + changes.anchoring ?? cost.anchoring, + ); + if (changes.amount) { resolved.amount = roundMoneyValue(draft, changes.amount); }