fix/bf-rollback
needs attentionviewing older commitb62fb9a · incrementalPR #259reviewed 2026-07-03 21:04 UTC3H · 4M · 5L · 4I- Purpose
- Establish a safe, reviewed rollback path before running the derived-concept backfill on production; fixes a real --rpu array-binding bug surfaced on staging.
- Goal
- Extract shared helpers into a SSOT module to prevent drift between backfill and rollback scripts.
- Sub-goals
- SG-1: New rollback script that is the exact inverse of the backfill
- SG-2: Fix --rpu array serialization bug (sql.join scalar params vs ANY(jsArray))
- SG-3: Extract shared helpers into scripts/lib/derived-bill-concepts.ts
- What
- New shared lib module (121 lines); backfill slimmed by extracting helpers (-74 lines); rollback refactored to use shared lib and now wraps COUNT+UPDATE in a single transaction.
- Why
- Prevents guard logic and SQL fragments from diverging; DERIVED_CONCEPT_KEYS constant ensures scan filter and strip predicate always target the same keys.
- Areas
- scripts/lib/derived-bill-concepts.ts+121−0scripts/backfill-derived-bill-concepts.ts+10−74scripts/rollback-derived-bill-concepts.ts+86−0
- Blast
- 3 files, +217/−74 lines; scripts/ only — no application code, no domain changes, no DB migrations.
Findings · 16
correctness3
countMatching closure captures db not tx — latent refactor trap
scripts/rollback-derived-bill-concepts.ts:40
Works correctly in dry-run path today. Calling inside the transaction would read from a separate connection. Thread tx as a parameter or inline both paths.
Transaction isolation comment overstates READ COMMITTED guarantee
scripts/rollback-derived-bill-concepts.ts:48
Drizzle issues plain BEGIN (READ COMMITTED). COUNT and UPDATE are separate snapshots; concurrent writes can still cause drift. Comment should not imply otherwise.
keyList NOT IN silently drops jsonb items with null 'key' field
scripts/lib/derived-bill-concepts.ts:117
li->>'key' NOT IN evaluates to NULL when key is SQL NULL, stripping those items. Add OR li->>'key' IS NULL. Pre-existing bug, good fix opportunity.
security2
maskedUrl redacts password but not username
scripts/lib/derived-bill-concepts.ts:57
Low risk for Supabase conventions but worth noting.
All SQL values properly parameterized via Drizzle
scripts/lib/derived-bill-concepts.ts:97
No SQL injection risk.
conventions2
as const on DERIVED_CONCEPT_KEYS unused by consumers
scripts/lib/derived-bill-concepts.ts:14
Consumers accept readonly string[]. Drop as const or tighten parameter types.
File-level JSDoc restates the filename
scripts/lib/derived-bill-concepts.ts:1
tests5
No tests for guardConnection — primary safety gate, now shared
scripts/lib/derived-bill-concepts.ts:49
Extracting to shared module makes it a single point of failure. Unit tests with vi.spyOn(process, 'exit') cover ref mismatch, missing URL, malformed URL.
No tests for parseRpus — prior --rpu bug history
scripts/lib/derived-bill-concepts.ts:27
Both comma-separated and repeated-flag forms need coverage. Pure unit test, zero infra.
No tests for SQL-generating functions — wrong SQL silently touches all rows
scripts/lib/derived-bill-concepts.ts:97
rpuInFilter/hasAnyKeyFilter/keyList are pure functions. Empty array edge case must not produce IN ().
No tests for parseLimit
scripts/lib/derived-bill-concepts.ts:41
Cover: missing flag, valid int, non-numeric, negative, float, zero.
No tests for maskedUrl — credential leakage if masking regresses
scripts/lib/derived-bill-concepts.ts:57
If masking regresses, POSTGRES_URL password appears in logs.
improvement4
maskedUrl strips query string, hiding connection options from logs
scripts/lib/derived-bill-concepts.ts:67
u.search = '' removes sslmode etc. Query string has no secrets; only password needs redacting.
countMatching helper only used in dry-run path, inlined in live path
scripts/rollback-derived-bill-concepts.ts:43
Asymmetry invites misreading. Thread tx or inline both.
argValue/parseRpus/parseLimit read process.argv directly — untestable without mutation
scripts/lib/derived-bill-concepts.ts:21
Accept optional argv parameter defaulting to process.argv.
rpuInFilter returns empty sql`` on no RPUs — implicit contract
scripts/lib/derived-bill-concepts.ts:97