fix(scripts): make the sql Date-binding audit precise and crash-proof - #6340
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryLow Risk Overview The Date audit now ties tagged templates to Reviewed by Cursor Bugbot for commit 7d05018. Configure here. |
Greptile SummaryThe PR makes the SQL Date-binding audit resolve actual Drizzle bindings, track Date values by function scope, tolerate parser failures, support multiline suppression annotations, and scan root scripts. It also removes script-only unit tests and their test-oriented exports while keeping the standalone repository checks runnable.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| scripts/check-sql-date-binding.ts | Reworks tag resolution, Date-binding scope analysis, annotation handling, parse recovery, and scan coverage without an eligible blocking issue. |
| package.json | Removes references to deleted script tests while preserving direct execution of the production checks. |
| scripts/check-migrations-safety.ts | Makes the migration linter internal after deleting its only external test consumer. |
| scripts/check-tool-request-boundary.ts | Makes the boundary-analysis helper internal after deleting its only external test consumer. |
Reviews (5): Last reviewed commit: "chore(scripts): drop the script unit tes..." | Re-trigger Greptile
Resolve the drizzle `sql` tag from its import binding, scope Date bindings lexically, tolerate unparseable files, accept the allow annotation above a multi-line template, and scan the root scripts directory.
4aba547 to
d828a11
Compare
|
@cursor review |
|
@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 78cea08. Configure here.
|
@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 7d05018. Configure here.
scripts/check-sql-date-binding.ts(shipped in #6337) catches a real production bug class — a bareDateinterpolated into a drizzle rawsqltemplate reaches the postgres driver unserialized and throwsERR_INVALID_ARG_TYPE. The detection goal is kept unchanged. This PR fixes five ways the detector could fail CI on correct code, plus three false negatives that fall out of the same fix.False positives fixed
1. The tag was matched by the bare identifier name
sql. That cannot tell drizzle'ssqltag from postgres-js's own client tag (const sql = postgres(url)), which serializes Dates correctly. Two live call sites already sit in scanned directories:apps/sim/app/api/tools/postgresql/utils.ts:20—const sql = postgres({...})with **6 live interpolatingsql\`` templates** (~lines 233/241/255/268/280/307). Adding aDate` there is correct code the old detector would reject.packages/db/scripts/reconcile-workspace-storage.ts:21— same pattern; passed only by luck (its queries are literal-only).scripts/setup/probes.ts:24— a third, in the directory this PR newly scans.The tag is now resolved to an actual
import … from 'drizzle-orm'binding. A locally declaredconst sql = postgres(...)is no longer treated as the drizzle tag.2.
collectDateNamesover-approximated scope file-wide. Two sub-problems:TSPropertySignaturefields typedDatewere absorbed, so a singleinterface R { start: Date }marked the identifierstartas a Date for the whole file.const now = new Date()in one function made${now}in another — wherenowis a number — a violation.The audit found 97 of 179 (54%) files containing an interpolating
sqltemplate already bind at least one Date-typed name, across 282 distinct names dominated by exactly the collision-prone ones (now335 bindings,timestamp123,createdAt83,date75,updatedAt70,start24,expiresAt24,end22,cutoff15). A drizzle-scoped re-measure on this branch gives 88/142 files. Either way this was a when-not-if CI break.Bindings are now tracked in a lexical scope chain (function-level), with declaration-order-independent fix-point resolution retained for
const b = achains. Destructured Date params —function q({ since }: { since: Date })— were previously caught via theTSPropertySignaturepath; that true positive is preserved by handling destructuring patterns explicitly, and now also resolves through a named interface (function q({ since }: Range)).3. A parse failure crashed the run.
errorRecovery: truedoes not cover missing Babel plugins, andmain()had no try/catch, so a decorator, aclass accessor, JSX in a.tsfile, or any pre-existing syntax error failed CI with a stack trace. The per-fileparse()is now wrapped; an unparseable file is reported as visibly skipped (printed to stderr with the parser message and a count), not silently swallowed. Thedecoratorsplugin is added.4. The
// sql-date-bound: <reason>escape hatch was unusable for multi-line templates. It only inspected the line above the interpolated expression, which for a multi-line template sits inside the SQL string — the marker would be sent to Postgres as junk. This is the shape of the actual outage site (cleanup-stale-executions/route.ts'sCASE…ENDblock), so the escape hatch did not work where it was most needed. The annotation is now also accepted above the enclosingTaggedTemplateExpressionor its enclosing statement. An empty reason is still rejected at every anchor.5. Coverage hole:
SCAN_DIRSwas[apps, packages]; the rootscripts/directory was never scanned. Added (12,899 → 12,977 files).False negatives closed (free, from fix 1)
Aliased imports (
import { sql as raw }), namespace imports (d.sql\…`), and the matchingd.sql.param(...)` member form are now detected. All were previously invisible.Verification
Each fix has a test that is red before the change and green after — verified by reverting each fix individually and watching the specific test fail (6 reverts, 6 targeted failures).
End-to-end on the full repo:
sql.param(now, asyncJobs.startedAt)atcleanup-stale-executions/route.ts:250is reported at line 254.Dateinjected into the postgres-js templates inapps/sim/app/api/tools/postgresql/utils.tsorscripts/setup/probes.ts.nowshadow or aninterface { start: Date }file. Injecting both into the realapps/sim/background/cleanup-logs.ts: the old detector reports 2 violations, the new one reports 0.Runtime: 5.46s → 4.07s over a larger file set (12,899 → 12,977 files). Skipping files with no
drizzle-ormimport before the scope pass more than pays for the extra analysis.Known limitations (unchanged or accepted)
These are not covered — this PR does not claim full coverage:
import { CUTOFF } from './constants').${getCutoff()}.${dates[0]},${row.startedAt},${this.now}.isDateExpressiononly resolves bare identifiers and class fields are read asthis.x, collecting them could only ever produce false positives, never a catch — so they are dropped deliberately.Follow-up in this PR: script unit tests removed
Per review discussion, the repo does not carry unit tests for
scripts/— only 3 of 34 scripts had them. This PR now also removes all three, so the convention is consistent:scripts/check-sql-date-binding.test.ts,scripts/check-migrations-safety.test.ts, andscripts/check-tool-request-boundary.test.tsbun test …half of thecheck:sql-date-bindingandcheck:tool-request-boundarygates inpackage.json— both invoked files this PR deletes, so CI would fail otherwise.check:migrationsnever referenced its testfindSqlDateBindingViolations(a pure test wrapper) and un-exportsanalyzeSource,SCAN_DIRS,lintSql, andfindToolRequestBoundaryViolations— every one had zero references outside its own script and existed only so the tests could reach themAll three gates verified running and passing standalone afterwards:
The detector still reports the original outage site when its
sql.paramis reverted, and no YAML/JSON/script reference to any deleted file remains. Supersedes #6343, which is closed.