fix(db): bind every raw-sql Date through its column encoder - #6337
Conversation
`drizzle()` overwrites postgres-js's temporal serializers (OIDs 1082/1083/ 1114/1184/1182/1185/1115/1231) with an identity function because drizzle maps timestamps itself through the column's `mapToDriverValue`. A raw `sql` template carries no column context, so an interpolated `Date` skips that mapping, reaches the identity serializer unchanged, and the wire encoder throws `ERR_INVALID_ARG_TYPE`. The pools' `prepare` / `fetch_types` options are irrelevant: the serializer swap happens for all four combinations. Five live sites still interpolated a bare `Date`, the stale schedule-job filter among them — it has no try/catch, so a database async backend would surface a 500 from the schedule tick. Bind each cutoff with `sql.param(date, column)`. The testing `sql` mock's guard cannot see untested code or the tests that override the drizzle-orm mock, so add `check:sql-date-binding`: a Babel-AST audit over apps/** and packages/** that resolves Date-valued bindings per file and rejects any that reach a raw template unbound. Correct the mock's comment, which attributed the failure to postgres-js under `fetch_types: false`.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryMedium Risk Overview Production fixes switch four call sites to Prevention adds Tests add audit unit tests, new Reviewed by Cursor Bugbot for commit 06d8f38. Configure here. |
Greptile SummaryThe PR binds raw-SQL
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| scripts/check-sql-date-binding.ts | Adds the AST-based Date-binding audit and correctly restricts opt-out annotations to the documented comment form with a nonempty reason. |
| scripts/check-sql-date-binding.test.ts | Covers unbound Date detection, valid column-bound parameters, and malformed or incidental annotation markers. |
| apps/sim/app/api/schedules/execute/route.ts | Encodes the stale-job cutoff through the started-at timestamp column. |
| apps/sim/lib/data-drains/sources/cursor.ts | Encodes cursor timestamps through the caller-provided timestamp column. |
| apps/sim/lib/execution/remote-sandbox/image-registry.ts | Reuses one correctly encoded retention predicate for candidate selection and deletion claims. |
| apps/sim/lib/workspace-events/state.ts | Encodes the cooldown threshold through the last-fired-at timestamp column. |
| packages/testing/src/mocks/database.mock.ts | Clarifies the serialization failure and retains mock-level guards as a testing backstop. |
| .github/workflows/test-build.yml | Adds the SQL Date-binding audit to the test-build workflow. |
Reviews (2): Last reviewed commit: "fix(scripts): require the documented sql..." | Re-trigger Greptile
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 06d8f38. Configure here.
|
Post-merge correction — independent verification found three claims in this description are wrong. The fix itself is confirmed correct; these corrections make it look more valuable, not less. 1. 2. 3. "Failed on every run" is refuted as stated. 13 runs were scheduled in the window: 11 logged failures, 0 successes, and 2 logged nothing at all. The failure was also partial — the job still completed its Also: the production log contains no Postgres error — only Drizzle's Confirmed by the same verification: a live 2×2 matrix against real PostgreSQL proved Two follow-ups worth a separate PR (detector gaps, not regressions):
|
Why
#6327 fixed one instance of a
Dateinterpolated into a raw drizzlesqltemplate, but its stated mechanism was wrong and its coverage claim was incomplete.Real mechanism (verified).
drizzle()(drizzle-orm/postgres-js/driver) overwritesclient.options.serializersfor the temporal OIDs1082/1083/1114/1184/1182/1185/1115/1231with an identity function, because drizzle normally maps timestamps itself via the column'sPgTimestamp.mapToDriverValue. A rawsqltemplate carries no column context, so an interpolatedDateskips that mapping, hits the now-identity serializer unchanged, and the wire encoder throwsERR_INVALID_ARG_TYPE.A 2x2 matrix over
preparexfetch_typesshows the pool options are irrelevant — in all four combinations postgres-js's own serializer produces the ISO string beforedrizzle(), and a rawDateafter:Sites fixed
app/api/schedules/execute/route.ts(staleScheduleExecutionJobsFilter)getAsyncBackendType() === 'database', and has no try/catch — a throw propagates a 500 from the schedule tick.lib/data-drains/sources/cursor.ts(timeCursorPredicate)timestampCol.lib/execution/remote-sandbox/image-registry.ts(x2, candidate query + claim guard)beyondRetentionfragment reused by both.lib/workspace-events/state.ts(claimCooldown)setWhere; a throw here would be silently swallowed by the caller.Each now binds with
sql.param(date, table.column), matching the sibling usage incleanup-stale-executions.Detection that actually works
The
packages/testingsqlmock guard cannot be relied on: it is invisible to untested code, and 26 test files override thedrizzle-ormmock (two of the sites above had passing tests). Addedbun run check:sql-date-binding, following thecheck:tool-request-boundarypattern (Babel AST + exported pure function +bun:testunit test), wired intopackage.jsonand thetest-buildworkflow.It parses every
.ts/.tsxunderapps/**andpackages/**(12.9k files, ~10s), resolves the set ofDate-valued bindings per file to a fixed point (new Date(...),: Date/Date | nullannotations on variables, parameters and property signatures, andconst b = achains regardless of declaration order), then rejects:Dateinterpolated into asql`…`orsql<T>`…`templatesql.param(date)with no encoder argumentEscape hatch:
// sql-date-bound: <reason>on the preceding line. Not covered: aDatearriving through a cross-file call whose type is only known to the type checker (the pass is per-file), and non-Datevalues needing an encoder (arrays remain covered only by the test mock).Tests
scripts/check-sql-date-binding.test.ts— every unbound form the audit must reject, plus bound params, non-Dateinterpolations, a non-sqltag, and the annotation.lib/workspace-events/state.test.ts— new file;claimCooldownhad no coverage at all.lib/data-drains/sources/cursor.test.ts—timeCursorPredicatewas untested.app/api/schedules/execute/route.test.tsandimage-registry.test.tsnow composecreateMockSql()instead of hand-rolledsqlstubs, so the shared guard applies to them.Red-then-green verified: reverting the
sql.paramat each of the four files turns the corresponding tests red (18 failures across schedules/image-registry, 4 across cursor/state) and the audit reports all five sites.Also corrected the misleading comment in
packages/testing/src/mocks/database.mock.ts, which blamed postgres-js underfetch_types: false, and noted the guard is a backstop rather than the gate.Verification
bun run check:sql-date-binding— cleancd apps/sim && bunx tsc --noEmit -p tsconfig.json— cleanbun run lint/bun run check— clean (9 pre-existing warnings inzoho-desk.test.ts)