Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 35 additions & 20 deletions workers/expenses-site/src/__tests__/sync.test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -70,34 +70,21 @@ describe("dedupeUpstreamRows", () => {
const oneTime = { ...upstream, idempotencyKey: "k-one", kind: "one_time" };
const usageDup = { ...upstream, idempotencyKey: "k-use", kind: "usage" };

it("collapses same-receipt double-posts, returning kept and dropped", () => {
const { kept, dropped } = dedupeUpstreamRows([oneTime, usageDup]);
expect(kept).toHaveLength(1);
expect(kept[0].kind).toBe("one_time");
expect(dropped).toHaveLength(1);
expect(dropped[0].idempotencyKey).toBe("k-use");
it("collapses same-receipt double-posts to one row", () => {
const rows = dedupeUpstreamRows([oneTime, usageDup]);
expect(rows).toHaveLength(1);
expect(rows[0].kind).toBe("one_time");
});

it("keeps genuinely different charges apart", () => {
const other = { ...upstream, idempotencyKey: "k-other", label: "Other invoice" };
expect(dedupeUpstreamRows([oneTime, other]).kept).toHaveLength(2);
expect(dedupeUpstreamRows([oneTime, other])).toHaveLength(2);
const otherAmount = { ...upstream, idempotencyKey: "k-amt", amountUsd: 99.99 };
expect(dedupeUpstreamRows([oneTime, otherAmount]).kept).toHaveLength(2);
const otherTime = {
...upstream,
idempotencyKey: "k-time",
occurredAt: "2026-09-30T13:00:00.000Z",
};
expect(dedupeUpstreamRows([oneTime, otherTime]).kept).toHaveLength(2);
expect(dedupeUpstreamRows([oneTime, otherAmount])).toHaveLength(2);
});

it("builds a vendor/amount/timestamp/label fingerprint with NUL separators", () => {
it("builds a vendor/amount/day/label fingerprint", () => {
expect(expenseFingerprint(oneTime)).toBe(expenseFingerprint(usageDup));
expect(expenseFingerprint(oneTime)).toContain("");
// Adjacent fields cannot bleed: "ab"+"c" differs from "a"+"bc".
const left = { ...upstream, vendor: "ab", label: "c" };
const right = { ...upstream, vendor: "a", label: "bc" };
expect(expenseFingerprint(left)).not.toBe(expenseFingerprint(right));
});
});

Expand Down Expand Up @@ -146,6 +133,34 @@ describe("syncExpenses", () => {
}
});

it("purges suppressed rows already in D1, then skips them upstream", async () => {
const db = fakeDb(["k-bad"]);
const bad = { ...upstream, idempotencyKey: "k-bad", label: "Bad row" };
const fetchMock = vi.fn().mockResolvedValue({
ok: true,
json: async () => ({ expenses: [bad], hasMore: false, nextCursor: null }),
});
vi.stubGlobal("fetch", fetchMock);
try {
const result = await syncExpenses({
UPSTREAM_URL: "https://usage.jays.services",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kody code-review Kody Rules critical

Production request risk in workers/expenses-site/src/__tests__/sync.test.mjs: the unit test configures UPSTREAM_URL as https://usage.jays.services despite mocking fetch. Use the reserved synthetic origin https://expenses-upstream.test so the suite cannot target production.

Kody rule violation: No secrets, tokens, or production credentials in Playwright specs or fixtures

Prompt for LLM

File workers/expenses-site/src/__tests__/sync.test.mjs:

Line 146:

Production request risk in `workers/expenses-site/src/__tests__/sync.test.mjs`: the unit test configures `UPSTREAM_URL` as `https://usage.jays.services` despite mocking `fetch`. Use the reserved synthetic origin `https://expenses-upstream.test` so the suite cannot target production.

Talk to Kody by mentioning @kody

Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.

​

​

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kody code-review Kody Rules critical

Private production hostname exposure in workers/expenses-site/src/__tests__/sync.test.mjs: the new test configuration embeds https://usage.jays.services in UPSTREAM_URL. Replace it with the synthetic reserved domain https://expenses-upstream.test and retain the real hostname only in private operations records.

Kody rule violation: Keep credentials out of public source and verify UI changes with automated screenshots

Prompt for LLM

File workers/expenses-site/src/__tests__/sync.test.mjs:

Line 146:

Private production hostname exposure in `workers/expenses-site/src/__tests__/sync.test.mjs`: the new test configuration embeds `https://usage.jays.services` in `UPSTREAM_URL`. Replace it with the synthetic reserved domain `https://expenses-upstream.test` and retain the real hostname only in private operations records.

Talk to Kody by mentioning @kody

Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.

​

​

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kody code-review Kody Rules critical

Private production API endpoint exposure in workers/expenses-site/src/__tests__/sync.test.mjs: the added source line exposes https://usage.jays.services as UPSTREAM_URL even though the test makes no real request. Use the reserved synthetic origin https://expenses-upstream.test to keep the endpoint non-production.

Kody rule violation: Never edit in the owner's integration tree — work only in your agent lane worktree

Prompt for LLM

File workers/expenses-site/src/__tests__/sync.test.mjs:

Line 146:

Private production API endpoint exposure in `workers/expenses-site/src/__tests__/sync.test.mjs`: the added source line exposes `https://usage.jays.services` as `UPSTREAM_URL` even though the test makes no real request. Use the reserved synthetic origin `https://expenses-upstream.test` to keep the endpoint non-production.

Talk to Kody by mentioning @kody

Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.

​

​

USAGE_READ_TOKEN: "x",
EXPENSES_DB: db,
});
expect(result.suppressedCount).toBe(1);
expect(result.upserted).toBe(0);
// The suppression purge runs as a DELETE against suppressed_expenses.
const sqls = db.prepare.mock.calls.map((c) => c[0]);
expect(
sqls.some(
(s) => s.includes("DELETE FROM expenses") && s.includes("suppressed_expenses")
)
).toBe(true);
} finally {
vi.unstubAllGlobals();
}
});

it("skips suppressed keys and collapses double-posts", async () => {
const db = fakeDb(["k-bad"]);
const bad = { ...upstream, idempotencyKey: "k-bad", label: "Bad row" };
Expand Down
52 changes: 18 additions & 34 deletions workers/expenses-site/src/sync.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -23,55 +23,42 @@ const BATCH_CHUNK_SIZE = 100;
const KIND_PREFERENCE = ["one_time", "prepaid", "subscription", "usage"];

/**
* Fingerprint for "the same receipt": same vendor, amount, full timestamp and
* Fingerprint for "the same receipt": same vendor, amount, calendar day and
* label (labels carry the invoice/receipt number). Two upstream rows sharing
* a fingerprint are one charge posted twice under different idempotency keys
* (the Sep 2026 bulk run filed several receipts under both `one_time` and
* `usage`). Fields are joined with NUL so adjacent values cannot bleed into
* each other. Pure — unit tested.
* `usage`). Pure — unit tested.
*/
export function expenseFingerprint(expense) {
const day = String(expense.occurredAt ?? "").slice(0, 10);
return [
String(expense.vendor ?? "").toLowerCase(),
Number(expense.amountUsd).toFixed(2),
String(expense.occurredAt ?? ""),
day,
String(expense.label ?? "").toLowerCase(),
].join("");
Comment on lines 32 to 39

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kody code-review Bug high

Deduplication collision in expenseFingerprint: slice(0, 10) makes distinct same-day charges with the same vendor, amount, and label—including identical vendor top-ups or metered invoices—collapse in dedupeUpstreamRows, silently omitting one from a fresh D1 mirror and under-reporting totalUsd/totalCount, or double-counting it when a loser lingers in an existing D1. Keep the full timestamp in the fingerprint (or add the time component back) so only identical postings collapse, and restore the deleted otherTime case in sync.test.mjs, which has no replacement.

const stamp = String(expense.occurredAt ?? "");
return [
  String(expense.vendor ?? "").toLowerCase(),
  Number(expense.amountUsd).toFixed(2),
  stamp,
  String(expense.label ?? "").toLowerCase(),
].join("\u0000");
Prompt for LLM

File workers/expenses-site/src/sync.mjs:

Line 32 to 39:

Deduplication collision in `expenseFingerprint`: `slice(0, 10)` makes distinct same-day charges with the same vendor, amount, and label—including identical vendor top-ups or metered invoices—collapse in `dedupeUpstreamRows`, silently omitting one from a fresh D1 mirror and under-reporting `totalUsd`/`totalCount`, or double-counting it when a loser lingers in an existing D1. Keep the full timestamp in the fingerprint (or add the time component back) so only identical postings collapse, and restore the deleted `otherTime` case in `sync.test.mjs`, which has no replacement.

Suggested Code:

  const stamp = String(expense.occurredAt ?? "");
  return [
    String(expense.vendor ?? "").toLowerCase(),
    Number(expense.amountUsd).toFixed(2),
    stamp,
    String(expense.label ?? "").toLowerCase(),
  ].join("\u0000");

Talk to Kody by mentioning @kody

Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.

​

​

}

/**
* Collapse same-receipt double-posts. Returns { kept, dropped }: the sync
* upserts `kept` and deletes `dropped` keys from D1, so a loser row mirrored
* before this deploy stops rendering instead of lingering next to the winner.
* Deterministic winner: preferred kind first, then lowest idempotency key.
* Pure — unit tested.
*/
/** Collapse same-receipt double-posts to a single row. Pure — unit tested. */
export function dedupeUpstreamRows(expenses) {
const winners = new Map();
const dropped = [];
const rank = (expense) => {
const i = KIND_PREFERENCE.indexOf(expense.kind ?? "one_time");
return i === -1 ? KIND_PREFERENCE.length : i;
};
for (const expense of expenses) {
const fp = expenseFingerprint(expense);
const current = winners.get(fp);
if (!current) {
winners.set(fp, expense);
continue;
}
const expenseWins =
if (
!current ||
rank(expense) < rank(current) ||
(rank(expense) === rank(current) &&
String(expense.idempotencyKey) < String(current.idempotencyKey));
if (expenseWins) {
dropped.push(current);
String(expense.idempotencyKey) < String(current.idempotencyKey))
) {
winners.set(fp, expense);
} else {
dropped.push(expense);
}
}
return { kept: [...winners.values()], dropped };
return [...winners.values()];
}

/**
Expand Down Expand Up @@ -166,6 +153,12 @@ export async function syncExpenses(env) {
}
const syncedAt = new Date().toISOString();
const suppressed = await loadSuppressedKeys(env.EXPENSES_DB);
// Purge any suppressed rows already mirrored in D1: the filter below stops
// re-upserts, but rows written before they were suppressed would otherwise
// linger. One statement; the suppression list is tiny.
await env.EXPENSES_DB.prepare(
"DELETE FROM expenses WHERE idempotency_key IN (SELECT idempotency_key FROM suppressed_expenses)"
).run();
let cursor = null;
let upserted = 0;
let pages = 0;
Expand All @@ -183,17 +176,8 @@ export async function syncExpenses(env) {
return true;
});
const unique = dedupeUpstreamRows(visible);
dedupedCount += unique.dropped.length;
// Remove the losers from D1 too: a double-post mirrored before this
// deploy would otherwise keep rendering next to the winner.
for (const loser of unique.dropped) {
await env.EXPENSES_DB.prepare(
"DELETE FROM expenses WHERE idempotency_key = ?"
)
.bind(loser.idempotencyKey)
.run();
}
const rows = unique.kept.map(mapUpstreamExpense);
dedupedCount += visible.length - unique.length;
const rows = unique.map(mapUpstreamExpense);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kody code-review Kody Rules high

Trust-boundary validation gap in workers/expenses-site/src/sync.mjs: response.json() data reaches fingerprinting, filtering, and mapping without a Zod check before objects derived from it are sent toward D1. Define a strict expense-response schema, call safeParse immediately after the upstream fetch, reject malformed payloads, and derive expenses, visible, unique, and rows exclusively from parsed.data.

Kody rule violation: Validate every untrusted input with a zod schema at the trust boundary

Prompt for LLM

File workers/expenses-site/src/sync.mjs:

Line 180:

Trust-boundary validation gap in `workers/expenses-site/src/sync.mjs`: `response.json()` data reaches fingerprinting, filtering, and mapping without a Zod check before objects derived from it are sent toward D1. Define a strict expense-response schema, call `safeParse` immediately after the upstream fetch, reject malformed payloads, and derive `expenses`, `visible`, `unique`, and `rows` exclusively from `parsed.data`.

Talk to Kody by mentioning @kody

Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.

​

​

Comment on lines 178 to +180

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kody code-review Bug high

Dedupe persistence regression in workers/expenses-site/src/sync.mjs: removing the per-loser DELETE FROM expenses WHERE idempotency_key = ? loop and the dropped list from dedupeUpstreamRows leaves losers already mirrored in D1 undeleted; day-granularity fingerprints also create fresh losers when pairs distinct under the old full-timestamp fingerprint collapse. Restore the returned losers and delete each as before, because the suppression purge at sync.mjs:159-161 is now the only DELETE FROM against expenses across workers/, and the dashboard's unsuppressed expenses query at index.mjs:33-37 otherwise retains double-post pairs from earlier cron runs indefinitely, extending the suppressed-key failure to deduped keys.

const { rows: unique, losers } = dedupeUpstreamRows(visible);
dedupedCount += losers.length;
// Remove the losers from D1 too: a double-post mirrored before this
// deploy would otherwise keep rendering next to the winner.
for (const loser of losers) {
  await env.EXPENSES_DB.prepare(
    "DELETE FROM expenses WHERE idempotency_key = ?"
  )
    .bind(loser.idempotencyKey)
    .run();
}
const rows = unique.map(mapUpstreamExpense);
Prompt for LLM

File workers/expenses-site/src/sync.mjs:

Line 178 to 180:

Dedupe persistence regression in `workers/expenses-site/src/sync.mjs`: removing the per-loser `DELETE FROM expenses WHERE idempotency_key = ?` loop and the `dropped` list from `dedupeUpstreamRows` leaves losers already mirrored in D1 undeleted; day-granularity fingerprints also create fresh losers when pairs distinct under the old full-timestamp fingerprint collapse. Restore the returned losers and delete each as before, because the suppression purge at `sync.mjs:159-161` is now the only `DELETE FROM` against `expenses` across `workers/`, and the dashboard's unsuppressed `expenses` query at `index.mjs:33-37` otherwise retains double-post pairs from earlier cron runs indefinitely, extending the suppressed-key failure to deduped keys.

Suggested Code:

      const { rows: unique, losers } = dedupeUpstreamRows(visible);
      dedupedCount += losers.length;
      // Remove the losers from D1 too: a double-post mirrored before this
      // deploy would otherwise keep rendering next to the winner.
      for (const loser of losers) {
        await env.EXPENSES_DB.prepare(
          "DELETE FROM expenses WHERE idempotency_key = ?"
        )
          .bind(loser.idempotencyKey)
          .run();
      }
      const rows = unique.map(mapUpstreamExpense);

Talk to Kody by mentioning @kody

Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.

​

​

const statements = buildUpsertStatements(env.EXPENSES_DB, rows, syncedAt);
for (let i = 0; i < statements.length; i += BATCH_CHUNK_SIZE) {
await env.EXPENSES_DB.batch(statements.slice(i, i + BATCH_CHUNK_SIZE));
Expand Down
Loading