fix(import): close three validation gaps found in review round 1

I3: purchase.source: null (and a null entry in recurring[]) reached a bare
property access and threw a raw TypeError instead of ImportValidationError.
null is not undefined, so the existing `!== undefined` guard let it through;
source: null is a plausible hand-edit since the sibling field on the same
purchase literal is schedule: null. Both paths now check for null/non-object
explicitly and refuse with the file's own wording, matching the defensive
style already used two lines away (cost.amount?.currency,
schedule?.slices ?? []).

I1/I2: splitMode and splits[].value were unchecked on both purchases and
recurring costs. An unrecognised splitMode silently takes charges.ts's
"fixed" branch instead of "percent" (or vice versa), turning an intended
percentage into an absolute amount with no error. A non-finite split value
propagates as NaN through distributeByWeight/distributeByAmount, and
roundMoney(NaN, digits) returns NaN rather than throwing. Both are now
checked in one new requireSplits() helper shared by the purchases loop and
the recurring-costs loop, replacing each loop's separate copy of the
no-splits/one-absorber/unknown-post checks rather than adding a third copy.
Per AGENTS.md §3, requireSplits only rejects non-numbers: negative values,
values over 100, and splits that don't sum to the total stay deliberately
unchecked.
This commit is contained in:
Lucas Winther
2026-09-03 18:24:25 +02:00
parent 74ae4b21e7
commit 65543eb37b
2 changed files with 153 additions and 31 deletions
+71
View File
@@ -347,6 +347,42 @@ describe("recurring cost validation", () => {
expect(() => parseDatasetJson(withRecurring([{ ...valid, splits: [] }])))
.toThrow(/no splits/);
});
test("a null recurring cost entry is refused, not a raw TypeError", () => {
// `recurring: [null]` used to reach `cost.id`/`cost.name` unguarded and
// throw TypeError instead of the file's own ImportValidationError.
expect(() => parseDatasetJson(withRecurring([null]))).toThrow(ImportValidationError);
});
test("an unknown splitMode is refused", () => {
// charges.ts picks distributeByWeight/distributeByAmount on
// `splitMode === "percent"`; any other string silently takes the "fixed"
// branch and treats a percentage as an absolute amount.
expect(() => parseDatasetJson(withRecurring([{ ...valid, splitMode: "ratio" }])))
.toThrow(/splitMode/);
});
test("a non-numeric split value is refused", () => {
// A non-finite value would otherwise propagate as NaN through the fold,
// and roundMoney(NaN, digits) returns NaN rather than throwing.
expect(() =>
parseDatasetJson(withRecurring([{
...valid,
splits: [{ postId: "housing", value: "oops", absorbsRemainder: true }],
}])),
).toThrow(/finite number/);
});
test("a negative split value is still accepted", () => {
// Deliberately unchecked per AGENTS.md §3: a refund is a normal line.
// Only non-numbers are rejected, never a negative or over-100 value.
expect(() =>
parseDatasetJson(withRecurring([{
...valid,
splits: [{ postId: "housing", value: -50, absorbsRemainder: true }],
}])),
).not.toThrow();
});
});
describe("Purchase.source validation", () => {
@@ -391,4 +427,39 @@ describe("Purchase.source validation", () => {
}];
expect(() => parseDatasetJson(JSON.stringify(data))).toThrow(/occurrence date/);
});
test("a source of null is refused, not a raw TypeError", () => {
// `source: null` used to reach `null.recurringId` unguarded — a plausible
// hand-edit, since the sibling field on the same object literal IS
// `schedule: null`. `source` is typed absent-or-object, never null, so
// this must fail as ImportValidationError, not TypeError.
expect(() => parseDatasetJson(withSource(null))).toThrow(ImportValidationError);
});
});
describe("purchase splits validation", () => {
// I1/I2 apply identically to purchases: the shared requireSplits() helper
// must not check purchases to a lower standard than recurring costs.
test("an unknown splitMode on a purchase is refused", () => {
const data: any = populated();
data.purchases[0].splitMode = "ratio";
expect(() => parseDatasetJson(JSON.stringify(data))).toThrow(/splitMode/);
});
test("a non-numeric split value on a purchase is refused", () => {
const data: any = populated();
data.purchases[0].splits[0].value = "oops";
expect(() => parseDatasetJson(JSON.stringify(data))).toThrow(/finite number/);
});
test("a negative split value on a purchase is still accepted", () => {
const data: any = populated();
data.purchases[0].splits[0].value = -70;
expect(() => parseDatasetJson(JSON.stringify(data))).not.toThrow();
});
test("a well-formed purchase still round-trips through the shared helper", () => {
const data = populated();
expect(parseDatasetJson(exportDatasetJson(data)).purchases.length).toBe(1);
});
});
+82 -31
View File
@@ -38,6 +38,7 @@ const PURCHASE_DATE = /^\d{4}-(0[1-9]|1[0-2])(-(0[1-9]|[12]\d|3[01]))?$/;
const ANCHORINGS = new Set(["calendar", "lastCharge"]);
const RECURRENCE_KINDS = new Set(["everyNMonths", "everyNDays", "everyNWeeks"]);
const SPLIT_MODES = new Set(["percent", "fixed"]);
/**
* `Currency` is an open string now, so this — not the type system — is what
@@ -154,6 +155,65 @@ function requireRecurrence(recurrence: any, label: string): void {
}
}
/**
* Shared by the purchases loop and the recurring-costs loop below — the only
* two places a `splitMode`/`splits` pair occurs, and both feed the identical
* `distributeByWeight`/`distributeByAmount` branch in `domain/charges.ts`.
* One helper means the two write paths cannot silently drift into checking
* to different standards; the two loops used to each carry their own copy of
* the "no splits" / "one absorber" / "unknown post" checks, so this replaces
* both rather than adding a third copy.
*
* `splitMode` must be one of the two known modes: `charges.ts` picks its
* distribution function with `splitMode === "percent" ? ... : ...`, so any
* OTHER string silently takes the "fixed" branch and treats an intended
* percentage as an absolute amount — a silently wrong number, not a throw.
*
* Every split's `value` must be a finite number. A non-finite value (missing,
* a string, NaN, Infinity) would otherwise propagate through
* `distributeByWeight`/`distributeByAmount` as NaN, and `roundMoney(NaN,
* digits)` returns NaN rather than throwing — it would corrupt a total
* silently, deep inside the fold.
*
* NEGATIVE values, values over 100, and splits that do not sum to the total
* are all deliberately UNCHECKED — AGENTS.md lists these as intentional
* (a refund is a normal line, and the absorbing split reconciles the sum by
* design). Only non-numbers are rejected here.
*/
function requireSplits(
splits: unknown,
splitMode: unknown,
label: string,
postIds: ReadonlySet<string>,
): void {
if (!SPLIT_MODES.has(splitMode as string)) {
throw new ImportValidationError(
`${label} has an unknown splitMode "${String(splitMode)}"`,
);
}
if (!Array.isArray(splits) || splits.length === 0) {
throw new ImportValidationError(`${label} has no splits; at least one split is required`);
}
const absorbers = (splits as Array<Record<string, unknown>>).filter(
(s) => s?.absorbsRemainder,
).length;
if (absorbers !== 1) {
throw new ImportValidationError(
`${label} has ${absorbers} splits flagged absorbsRemainder; exactly one is required`,
);
}
for (const split of splits as Array<Record<string, unknown>>) {
if (!postIds.has(split?.postId as string)) {
throw new ImportValidationError(`${label} references unknown post "${String(split?.postId)}"`);
}
if (typeof split?.value !== "number" || !Number.isFinite(split.value)) {
throw new ImportValidationError(
`${label} has a split value that is not a finite number: ${String(split?.value)}`,
);
}
}
}
/**
* Validates far enough that the fold cannot throw on the imported data: every
* split points at a real post, every purchase has exactly one remainder
@@ -248,9 +308,17 @@ export function parseDatasetJson(text: string): Dataset {
const recurring = Array.isArray(data.recurring) ? (data.recurring as any[]) : [];
dataset.recurring = recurring;
const recurringIds = new Set(recurring.map((cost: any) => cost.id));
// Optional chaining, not `.id` directly: a null/non-object entry must fall
// through to the loop below and fail there with an ImportValidationError,
// not throw a raw TypeError here while merely collecting ids.
const recurringIds = new Set(recurring.map((cost: any) => cost?.id));
for (const cost of recurring) {
if (!cost || typeof cost !== "object") {
throw new ImportValidationError(
`A recurring cost is missing or not an object: ${JSON.stringify(cost)}`,
);
}
const label = `Recurring cost "${cost.name}"`;
requireCurrency(cost.amount?.currency, label, defined);
@@ -267,20 +335,7 @@ export function parseDatasetJson(text: string): Dataset {
`${label} has an unknown anchoring "${String(cost.anchoring)}"`,
);
}
if (!Array.isArray(cost.splits) || cost.splits.length === 0) {
throw new ImportValidationError(`${label} has no splits; at least one split is required`);
}
const absorbers = cost.splits.filter((s: any) => s.absorbsRemainder).length;
if (absorbers !== 1) {
throw new ImportValidationError(
`${label} has ${absorbers} splits flagged absorbsRemainder; exactly one is required`,
);
}
for (const split of cost.splits) {
if (!postIds.has(split.postId)) {
throw new ImportValidationError(`${label} references unknown post "${split.postId}"`);
}
}
requireSplits(cost.splits, cost.splitMode, label, postIds);
}
for (const month of dataset.months) {
@@ -297,22 +352,7 @@ export function parseDatasetJson(text: string): Dataset {
if (!PURCHASE_DATE.test(purchase.date)) {
throw new ImportValidationError(`${label} has an invalid date "${purchase.date}"`);
}
if (purchase.splits.length === 0) {
throw new ImportValidationError(`${label} has no splits; at least one split is required`);
}
const absorbers = purchase.splits.filter((s) => s.absorbsRemainder).length;
if (absorbers !== 1) {
throw new ImportValidationError(
`${label} has ${absorbers} splits flagged absorbsRemainder; exactly one is required`,
);
}
for (const split of purchase.splits) {
if (!postIds.has(split.postId)) {
throw new ImportValidationError(
`${label} references unknown post "${split.postId}"`,
);
}
}
requireSplits(purchase.splits, purchase.splitMode, label, postIds);
for (const slice of purchase.schedule?.slices ?? []) {
if (!MONTH_ID.test(slice.month)) {
throw new ImportValidationError(`${label} has invalid slice month "${slice.month}"`);
@@ -321,6 +361,17 @@ export function parseDatasetJson(text: string): Dataset {
}
if (purchase.source !== undefined) {
// `null` is not `undefined`, so it reaches here without being "no
// source". The field is typed as absent-or-object, never null, so a
// null (or any other non-object) is malformed input, not a missing
// field — reject it cleanly rather than dereferencing it into a raw
// TypeError. `schedule?.slices ?? []` two blocks up is the same
// defensive style applied to a sibling field that IS legitimately null.
if (!purchase.source || typeof purchase.source !== "object") {
throw new ImportValidationError(
`${label} has a source that is not an object: ${JSON.stringify(purchase.source)}`,
);
}
if (!recurringIds.has(purchase.source.recurringId)) {
throw new ImportValidationError(
`${label} names unknown recurring cost "${purchase.source.recurringId}"`,