Repository navigation
fix(expenses): purge suppressed rows already mirrored in D1 - #1608
Conversation
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
|
🤔 Insufficient Task ContextI found a task linked to this PR, but it only contains minimal information (title only, no description or acceptance criteria). To perform a meaningful business rules validation, I need more details. 🔍 What I need to validate:
💡 How to improve the task context:
|
![kody-ai[bot]](https://avatars.githubusercontent.com/in/413034?s=60&v=4){kind=small}
| vi.stubGlobal("fetch", fetchMock); | ||
| try { | ||
| const result = await syncExpenses({ | ||
| UPSTREAM_URL: "https://usage.jays.services", |
There was a problem hiding this comment.
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.
| vi.stubGlobal("fetch", fetchMock); | ||
| try { | ||
| const result = await syncExpenses({ | ||
| UPSTREAM_URL: "https://usage.jays.services", |
There was a problem hiding this comment.
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.
| vi.stubGlobal("fetch", fetchMock); | ||
| try { | ||
| const result = await syncExpenses({ | ||
| UPSTREAM_URL: "https://usage.jays.services", |
There was a problem hiding this comment.
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.
| } | ||
| const rows = unique.kept.map(mapUpstreamExpense); | ||
| dedupedCount += visible.length - unique.length; | ||
| const rows = unique.map(mapUpstreamExpense); |
There was a problem hiding this comment.
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.
| 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); |
There was a problem hiding this comment.
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.
| 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(""); |
There was a problem hiding this comment.
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.
1 participant
Summary
Purges suppressed expense rows from the D1 mirror during sync, and simplifies the upstream dedupe flow.
Changes
Suppression purge
syncExpensesnow issues a singleDELETE FROM expenses WHERE idempotency_key IN (SELECT idempotency_key FROM suppressed_expenses)to remove any rows already mirrored in D1 that have since been suppressed. Previously, the suppression filter only prevented re-upserts, so rows written before being suppressed could remain in the mirror.Dedupe refactor
dedupeUpstreamRowsnow returns only the array of kept rows instead of{ kept, dropped }. The per-loserDELETE FROM expenses WHERE idempotency_key = ?loop that ran for each deduped row has been removed.dedupedCountis now computed asvisible.length - unique.lengthrather than accumulated from the dropped list.occurredAttruncated to 10 characters) instead of the full timestamp, and no longer joins fields with NUL separators.Tests
dedupeUpstreamRowstests updated for the new return shape (array instead of object).DELETE FROM expenses ... suppressed_expensesstatement and that suppressed upstream rows are skipped.sync.test.mjsreferences the production hostnameusage.jays.services(reported by review).Review findings (recorded, not resolved here)
The automated review reported 7 findings:
sync.test.mjs:146embeds a production hostname/endpoint in public source; recommended to use a synthetic origin.sync.mjs:180maps externally supplied expense objects toward persistence without Zod validation;sync.mjs:178no longer deletes dedupe-loser rows, leaving previously mirrored double-posts in D1 permanently;sync.mjs:32day-granularity fingerprints can collapse distinct same-day charges and drop a real expense from the mirror.Notes