Skip to content

[Tier 1 / Code] DRY and correctness — C1, C2, C3, C4 #86

Description

@brewpirate

Tracking issue for Tier 1 code-improvement work from the product review. Source: claude/product-review-recommendations-LXL1v plan at /root/.claude/plans/an-you-look-abundant-journal.md.

Suggested labels (not auto-applied): tier-1, code-improvement.

Items

- [ ] C1. Extract shared store base / mixin

~2,234 lines across the four packages/store-*/src/index.ts files have substantial structural duplication: insertRun, updateRun, insertFailure(s), schema-inspection helpers, row mapping, AbortSignal handling. Extract to packages/core/src/store/adapter-base.ts exposing a template-method base class (or a small set of higher-order helpers). Each backend then provides only the dialect-specific SQL and driver wiring.

  • Estimated reduction: 40–50%
  • Files: all four packages/store-*/src/index.ts

- [ ] C2. Validate driver row outputs at the boundary

Stores cast driver rows as as unknown as Record<string, unknown> then access fields with String()/Number() coercion (e.g., packages/store-sqlite/src/index.ts:466, :517). If a driver shape changes, this silently produces "undefined" strings or NaN. Apply the same parse() discipline used at insert boundaries: define an ArkType flakyPatternRowSchema and parse rows before mapping. The codebase already does this once at packages/store-sqlite/src/index.ts:432 — generalize it.

- [ ] C3. Stop swallowing fatal plugin errors

packages/plugin-bun/src/pending-writes.ts:33 and packages/plugin-bun/src/preload.ts:296,319 catch all errors and log.warn, even fatal ones (missing tables, bad credentials). For the Bun preload, distinguish fatal-on-startup (auth/migration failure → set process.exitCode=1, fail loud) from transient (retry with backoff, then drain-or-fail). Keeps quiet in the happy path; makes silent data loss impossible.

- [ ] C4. Plugin isolation: catch-all around test wrappers

packages/plugin-bun/src/preload.ts:248–269 and packages/plugin-vitest/src/index.ts:42–53 could let an exception in failure-recording (e.g., a malformed stack from a test wrapper) propagate into the host runner. Wrap recording paths in try/catch that logs and continues — plugin failures must never abort the test run being instrumented (also enforced by the plugin-isolated-failure.md rule in .claude/rules/).

Why this tier

DRY / correctness foundation. C1 is the largest single refactor in the review and pays for itself by making every future store-touching change ~4× cheaper. C2–C4 are small, safe correctness fixes that should bundle in one PR.

Verification

  • C1: full integration test suite must pass (Postgres + Turso in-memory + Supabase secrets)
  • C2: tests for malformed driver shape (mock driver returning unexpected types) → parse error, not silent coerce
  • C3: preload test where store throws on first call → process.exitCode === 1
  • C4: preload test where recording throws mid-test → host runner continues
  • bun test, bun run test:integration (where DBs available), bun run typecheck, bun run check clean

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions