GLOOK-41: translate DATE_ADD for SQLite — org report 500s when a report has spend data

[msogin]

Closes GLOOK-41.

GET /api/report/<id>/org returns 500 on SQLite whenever the report has a Claude Code spend window. The Org Summary page renders "Error: Internal Server Error" and no data.

Root cause

src/lib/report/org.ts:234 builds MySQL-only SQL for the spend window:

AND committed_at < DATE_ADD(?, INTERVAL 1 DAY)

translateSQL() in src/lib/db/sqlite.ts had rules for DATE_SUB(NOW(), INTERVAL n DAY) — which is why the six uses in projects/* work — but none for DATE_ADD. The statement reached better-sqlite3 verbatim:

SqliteError: near "1": syntax error
    at Object.execute (src/lib/db/sqlite.ts:344)
    at async getOrgReport (src/lib/report/org.ts:229)

Why it went unnoticed

The query sits behind if (ccStart && ccEnd && reportIds.length > 0). SQLite installs without spend data skip it entirely, and MySQL was never affected. But npm run seed populates a spend window, so the documented npm run seed then npm run dev:mock workflow hits it every time.

The existing suites referencing getOrgReport (report-org.test.ts, org-route-model-gating.test.ts, cc-apply-breakdowns.test.ts) mock the database, so they stayed green throughout.

Fix

Three rules mirroring the existing DATE_SUB pair:

• the datetime('now','localtime') form and the raw NOW() form first — NOW() is rewritten earlier in translateSQL, and its replacement contains a comma that would split a general pattern
• then the general expression form, which is what org.ts needs

The captured expression is spliced through verbatim, so a bound ? stays a bound parameter — the caller's value is never interpolated into SQL. There is a test asserting exactly that with a hostile string.

Tests

New src/lib/__tests__/unit/sqlite-date-add.test.ts drives the real SQLite driver via createSQLiteDB() rather than a mock — the whole point, since mocked tests are what let this through. Four cases: the translation works; the bound parameter stays bound; the org query's exclusive-end-of-day semantics hold at the boundary; and the NOW() variant survives the earlier rewrite.

All four were confirmed failing before the fix, with the same near "1": syntax error seen at runtime.

119 suites / 1140 tests / 9 snapshots green (was 118 / 1136). tsc --noEmit clean.

Verification

Reproduced against a seeded SQLite DB, then confirmed fixed end-to-end — GET /api/report/<id>/org now returns 200 with 8 developers, 16 model-usage rows, and a populated spend window. The Spend tab renders in mock mode.

Pre-existing since 1263c3c (2026-04-23). Unrelated to GLOOK-38; found while recording mock-mode footage for a demo video.

🤖 Generated with Claude Code

[msogin]

Multi-agent review — 4 independent reviewers

Reviewed by Senior Dev (holistic), Security Engineer (holistic), a minimalist scope gate, and the standard Smartling fullstack review. Reviewers did not see each other's findings. Every claim below was re-verified against real better-sqlite3 before posting. No fixes were applied — this is comment-only.

The fix is correct, and the central claim holds

translateSQL('… DATE_ADD(?, INTERVAL 1 DAY)') → datetime(?, '+1 days'), with the ? intact. I confirmed the rewrite is ?-count-preserving (no positional-parameter shift, which would have been the real cross-report data risk), that the function-form replacement keeps $&/$1 literal, and that no call site in the repo reaches translateSQL with attacker-influenced text inside the SQL string. Against the real stored committed_at format the translated clause returns the correct exclusive-end-of-day window, and org.ts has no other MySQL-isms — the endpoint is genuinely unblocked. Test 1 fails on revert. Suite: 1134 pass (dev-usage-card.test.tsx fails pre-existing, unrelated).

Highest-consensus finding

All four reviewers independently flagged that the "hostile string" test cannot fail — translateSQL never sees params, so interpolation is impossible by signature. The claim it's cited for is true; this test just isn't the evidence. Worth editing the PR description too, since it presents that test as the injection guard.

Three of four flagged sqlite.ts:414 as unreachable — :396 rewrites every NOW() first.

One finding only the Smartling review caught, and it's the one I'd fix before merge: :412 drops 'localtime', so NOW() and DATE_ADD(NOW(), …) disagree by the UTC offset (verified: 4h skew). The pre-existing DATE_SUB rules share it and do have live callers.

🔴 Rethink — the root cause under most of these findings

translateSQL is a whitelist rewriter with a passthrough default: anything it doesn't match reaches the driver verbatim and 500s at request time with a token-level parse error. This PR is the second iteration of that incident — a MySQL-ism ships, mocked suites stay green because they never call translateSQL, users find it as near "1": syntax error. Adding a fourth regex moves the boundary rather than closing it. Still unhandled: DATE_SUB(?, …) (no general form — the asymmetry is new as of this PR), INTERVAL n HOUR|MONTH|MINUTE, INTERVAL ? DAY, DATE_ADD(DATE(?), …).

Two cheap pieces would convert every future gap from a production 500 into a loud, greppable failure:

// end of translateSQL()
if (/\bINTERVAL\b|\bDATE_ADD\b|\bDATE_SUB\b/i.test(s)) {
  throw new Error(`translateSQL: untranslated MySQL date expression: ${sql}`);
}

plus a unit test scanning SQL template literals under src/lib/** for DATE_ADD|DATE_SUB|INTERVAL and asserting translateSQL leaves no INTERVAL behind — so new offenders fail CI, not production. translateSQL is pure; exporting it makes all of this testable without standing up a driver.

Open questions for the author

1. Scope — the reviewers genuinely disagree. The minimalist gate argues rule :412 and Tests 3–4 serve zero callers (grep -rn "DATE_ADD" → one non-test hit, org.ts:234; no DATE_ADD(NOW( anywhere) and should be deleted, leaving ~+40/−0 with identical regression protection. The other two reviewers propose fixing those same lines. Both are defensible — deleting is cheaper and the omitted case fails loudly; keeping preserves the DATE_SUB symmetry the file already uses. Your call, not mine.

2. Why DATE_ADD at all? endStr is already normalized to a bare YYYY-MM-DD by toIso() before binding (org.ts:229-234). Computing the exclusive end date in JS and binding it directly drops the dialect dependency from this query entirely — and makes the translator change unnecessary for the reported bug.

Deferred — real, but outside this diff

• Latent format mismatch. datetime() emits space-separated output; committed_at stores T-separated ISO-Z, compared bytewise. Verified ('2026-04-01T00:00:01Z' < '2026-04-01 12:00:00') → 0, though it's chronologically earlier. The current caller is safe only because its boundary lands on midnight. Any future non-midnight boundary returns wrong rows with no error. strftime('%Y-%m-%dT%H:%M:%SZ', …) would keep comparisons bytewise-ordered.
• DATE_SUB localtime bug — same as :412, but with live callers in untracked.ts, epic-stats.ts, epic-summary.ts.
• CLAUDE.md:54 still lists the translator as handling "INSERT IGNORE, ON DUPLICATE KEY UPDATE, and NOW()" — already missing LEFT and DATE_SUB, now DATE_ADD. That stale inventory is plausibly part of why this gap went unnoticed.

Assessment

Ready to merge? With fixes — the localtime bug at :412 and the dead rule at :414 are cheap now and neither has a production caller yet. Everything else is a suggestion or a follow-up. Both persona reviewers returned "safe to merge: yes" on the core change.

🤖 Generated with Claude Code

[msogin]

🟡 warning — this rule drops 'localtime', so NOW() and DATE_ADD(NOW(), …) disagree by the UTC offset

NOW() is rewritten at :396 to datetime('now','localtime'), but this rule emits datetime('now', '+N days') — no localtime. Verified against better-sqlite3 on a UTC-4 host:

NOW()                    -> 2026-08-18 16:27:20   (localtime)
DATE_ADD(NOW(), 0 DAY)   -> 2026-08-18 20:27:20   (UTC)

A single query mixing both gets two different "now"s, and on any non-UTC host a day-boundary window shifts by the offset.

(_match, days) => `datetime('now', 'localtime', '+${days} days')`

Note the pre-existing DATE_SUB rules at :402-406 carry the identical bug, and unlike this one they do have live callers — the 90-day windows in src/lib/projects/untracked.ts:195,245, epic-stats.ts:130,167, epic-summary.ts:187,220. Those are outside this PR's diff, but worth a follow-up.

[msogin]

🔵 suggestion — unreachable rule (flagged independently by 3 of 4 reviewers)

:396 globally replaces every NOW() before any DATE_ADD rule runs, so no NOW() token survives to reach this line. Probed across casings and whitespace shapes (DATE_ADD(NOW(),…), date_add(now(), …), spaced variants): the rule at :412 fires in every case, this one in none. The test at sqlite-date-add.test.ts:76 passes via :412, so nothing covers this rule.

The comment at :409-411 — "the NOW() forms get dedicated rules before the general one" — is true only of :412, and reads as if this line were load-bearing. The cited precedent, DATE_SUB at :405, is dead for the same reason ("Also handle case where NOW() hasn't been translated yet" describes a case that cannot occur), so the symmetry argument replicates a pre-existing wart rather than a working pattern.

Either delete :413-415, or move the NOW() replacement below the DATE_* rules so the ordering comment becomes real.

[msogin]

🟡 warning — passthrough default: everything this regex misses still 500s, silently accepted or not

The core rewrite is correct and I verified the parameterization claim holds (see the test-file comment). Two separate problems with the shape of the pattern:

1. It matches only integer literals, unit DAY, and paren/comma-free expressions. Everything else falls through verbatim and reproduces GLOOK-41 exactly. Verified non-matching — output identical to input:

DATE_ADD(DATE(x), INTERVAL 1 DAY)     DATE_ADD(?, INTERVAL ? DAY)
DATE_ADD(?, INTERVAL 1 MONTH)         DATE_ADD(?, INTERVAL 2 HOUR)

Also note DATE_SUB still has no general-expression form — the asymmetry with DATE_ADD is new as of this PR. The next person writing AND committed_at >= DATE_SUB(?, INTERVAL 7 DAY) reproduces this incident, and the mocked suites will stay green exactly as they did here.

2. The expression capture [^,()]+? is quote-blind, and the replacement injects ' characters. This is the first DATE_* rule whose capture isn't anchored to a literal NOW()/datetime(…), so it rewrites matching text inside string literals — and because it splices in '+N days', it terminates the enclosing literal and restructures the statement:

-- in:
WHERE msg = 'DATE_ADD(a' AND b = 'c , INTERVAL 9 DAY)' AND ts < DATE_ADD(?, INTERVAL 1 DAY)
-- out:
WHERE msg = 'datetime(a' AND b = 'c, '+9 days')'      AND ts < datetime(?, '+1 days')

Not reachable today — I traced every dynamic-SQL site (mcp/queries.ts, chat/tools.ts, report/org.ts, team-pulse/data.ts, teams/service.ts, jira/service.ts, report-runner/skip-classifier.ts) and every interpolation is a generated ? list or an allowlist-checked identifier. This is latent-class widening, not a live vulnerability. But it rests on an unwritten invariant ("no caller puts untrusted text in the SQL string") that nothing enforces, in a reporting app whose tables store commit messages and Jira summaries.

Anchoring to the shapes actually used closes both the literal case and keeps the fix:

s = s.replace(/DATE_ADD\s*\(\s*(\?|[A-Za-z_][A-Za-z0-9_.]*)\s*,\s*INTERVAL\s+(\d+)\s+DAY\s*\)/gi,
  (_match, expr, days) => `datetime(${expr}, '+${days} days')`);

Verified: still rewrites AND committed_at < DATE_ADD(?, INTERVAL 1 DAY) correctly, and leaves the hostile literal above untouched. Anything else falls through to a loud prepare() error, which is the right failure mode.

[msogin]

🔵 suggestion — predictable temp path, and four DB handles are never closed

path.join(os.tmpdir(), \glooker-glook41-${process.pid}-${Date.now()}.db`)is a guessable name in the shared temp dir, andnew Database(path)opens withO_CREATfollowing symlinks. Every sibling test already uses the safe primitive —cc-apply-breakdowns.test.ts:19, cc-breakdown-schema.test.ts:18, prompt-loader.test.ts:8, logger.test.ts:9all usefs.mkdtempSync`. This file is the only one that doesn't.

Separately, makeDb() at :37 opens a fresh handle per test (4 total), each re-running the full schema, ~20 ALTER migrations and seed inserts; none is closed, and afterAll then unlinks the file plus -wal/-shm with all four still open.

tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'glooker-glook41-'));
dbPath = path.join(tmpDir, 'test.db');
// afterAll, replacing the three-suffix unlink loop:
try { fs.rmSync(tmpDir, { recursive: true, force: true }); } catch { /* already gone */ }

Or set SQLITE_PATH=':memory:' as src/lib/__tests__/integration/cc-spend-end-to-end.test.ts:36 does, which removes the temp-file and WAL cleanup entirely. The env-var save/restore in this file is correct as written — keep it.

[msogin]

🟡 warning — this test cannot fail, and the PR description cites it as the injection guard

All four reviewers flagged this independently. translateSQL(sql: string): string receives only the SQL string — params are bound separately at sqlite.ts:352,362. Interpolating a bound ? is impossible by signature, not by care, so no change to translateSQL could make this test fail for the reason its name states.

What it actually asserts is that better-sqlite3 binds parameters and that SQLite's datetime() returns NULL on unparseable input — both properties of the driver, not of this diff.

To be clear, the underlying safety claim is true — I verified the rewrite is ?-count-preserving (the replacement introduces no ?, expr is spliced verbatim), so there's no positional-parameter shift, and the replacement being a function means $&/$1 inside expr stay literal. The problem is that this test isn't the evidence for it, while the genuinely untested surface — how translateSQL transforms adversarial SQL strings (see the sqlite.ts:419 comment) — has no direct test at all, because translateSQL isn't exported.

Two things worth doing:

• Rename to what it verifies: "returns NULL for an unparseable date rather than erroring", and drop the "rather than interpolating it" claim from the test name and the PR body.
• If the parameterization guarantee is worth asserting, export translateSQL and assert the rewrite directly: expect(translateSQL('… DATE_ADD(?, INTERVAL 1 DAY)')).toBe("… datetime(?, '+1 days')"). That's pure and far cheaper to test than standing up a real driver.

One caveat on the toBeNull() assertion: it enshrines silent-NULL as intended behavior. In org.ts:234 a NULL boundary makes committed_at < NULL unknown for every row, so the spend window returns empty with a 200 instead of an error. I confirmed that's fail-closed rather than widening (and the window is scoped by bound report_id IN (?,…), not by the date, so no report boundary can be crossed) — but it's worth being deliberate about.

[msogin]

🔵 suggestion — assertion is loose enough to miss a real dialect difference

toMatch(/^2026-03-19/) passes for any time-of-day. The thing worth pinning is that the boundary lands at exact midnight rather than carrying a time component — a genuine MySQL/SQLite difference. Confirmed: datetime('2026-03-18','+1 days') returns exactly 2026-03-19 00:00:00.

expect(String(rows[0].boundary)).toBe('2026-03-19 00:00:00');

This is the one test in the file that unambiguously earns its place — it drives the real driver, fails on revert, and pins the only rule with a live caller. Tightening it here also lets the separate exclusive-end-of-day test at :62 go away (see that comment).

[msogin]

🟣 question — does this test belong to this PR?

Two reviewers reached opposite conclusions, so flagging rather than asserting:

Against: the exclusive-end-of-day semantics come from the < operator in org.ts:234, which this PR doesn't touch, plus SQLite's TEXT comparison. It doesn't touch commit_analyses or getOrgReport — it's a synthetic SELECT 1 … WHERE — so it isn't integration coverage of the caller either. The one real thing it pins (midnight boundary) is better covered by tightening :48.

For: it's the only case here expressing the caller's actual semantics, and a dialect layer is a bad place to undertest.

Either way, the inputs are wrong. It uses space-separated timestamps, but production committed_at is GitHub's ISO-8601 2026-03-31T23:59:59Z (src/lib/github.ts:742 → src/lib/report-runner.ts:411), compared as TEXT against datetime()'s space-separated output. If you keep the test, use the real format:

expect(await inWindow('2026-03-31T23:59:59Z')).toBe(true);
expect(await inWindow('2026-04-01T00:00:00Z')).toBe(false);

I verified the real format still yields correct results here — 'T' (0x54) sorts above ' ' (0x20), which keeps the boundary exclusive because this boundary is midnight. That's luck, not design; see the summary for the latent case.

[msogin]

🔵 suggestion — this assertion can't detect a sign flip in the rule it covers

toMatch(/^\d{4}-\d{2}-\d{2}/) only checks the output looks like a date — it pins neither sign nor magnitude.

That matters specifically here: the rule at sqlite.ts:412 was written by copying the DATE_SUB block eight lines above, and flipping '+${days} days' to '-${days} days' is exactly the slip that copying invites. With that flip, datetime('now','-7 days') still matches this regex and all four tests stay green — the suite would ship a DATE_ADD that subtracts. Test 1 pins the sign, but for the general rule, a different code path.

The test also can't tell which of the two rules fired, so it doesn't verify what its name claims. Comparing both rewrites in one statement fixes both problems — and would currently fail, surfacing the localtime bug flagged at sqlite.ts:412:

`SELECT DATE_ADD(NOW(), INTERVAL 0 DAY) AS added, NOW() AS raw`
// expect(rows[0].added).toBe(rows[0].raw)

Counterpoint worth weighing: grep -rn "DATE_ADD" returns exactly one non-test hit — org.ts:234 — and there is no DATE_ADD(NOW( anywhere in the repo. If you take the parsimony route and drop rule :412, this test goes with it.

[msogin]

Thorough review — thanks. All findings addressed in 1b1cd84. Two of them changed my mind about the shape of the fix, and one of the suggested fixes turned out to be insufficient when I tested it.

119 suites / 1141 tests pass, tsc clean. (dev-usage-card.test.tsx and two tsc errors come from an uncommitted local deletion of usage-card.tsx in my working tree — it exists on main, and it's unrelated to this PR.)

Fixed

localtime at :412 — confirmed and measured. You were right, and it mattered more for DATE_SUB than for the line flagged. Measured on this host:

datetime('now','localtime')               2026-08-18 16:43:32
datetime('now', 'localtime', '-90 days')  2026-05-20 16:43:32   <- new
datetime('now', '-90 days')               2026-05-20 20:43:32   <- old, 4h skew

DATE_SUB now emits localtime, so it agrees with NOW(). That's the one with six live callers (projects/untracked.ts, epic-stats.ts, epic-summary.ts).

Dead rule at :414 — deleted rather than fixed, which also answers Q1. I took the minimalist gate's side. DATE_ADD(NOW( appears nowhere in the codebase (grep agrees with you: one non-test hit, org.ts:234), so both DATE_ADD(NOW(), …) rules served zero callers and carried the localtime skew. Deleting removes a bug and an unexercised code path in one move, and the guard below makes the omission loud instead of silently wrong. Keeping-and-fixing would have preserved symmetry with DATE_SUB at the cost of shipping more untested surface — and an unexercised rule is exactly where this skew hid in the first place.

Your fix for the string-literal case doesn't actually close it. I implemented the suggested anchor and wrote a test asserting the hostile literal was left alone — the test failed. 'DATE_ADD(a , INTERVAL 9 DAY)' still matches [A-Za-z_][A-Za-z0-9_.]* via the bare identifier a, so the rewrite fires inside the literal and the injected quotes restructure the statement exactly as you described. Anchoring narrows the class; it doesn't eliminate it.

So the rewrites now run per non-literal chunk (outsideStringLiterals(), doubled-quote aware). Verified:

in : SELECT 1 WHERE msg='DATE_ADD(q , INTERVAL 5 DAY)' AND t < DATE_ADD(?, INTERVAL 1 DAY)
out: SELECT 1 WHERE msg='DATE_ADD(q , INTERVAL 5 DAY)' AND t < datetime(?, '+1 days')

Literal byte-identical, real expression rewritten. The anchored capture is kept as well — belt and braces. DATE_SUB also gained the general form, so the asymmetry this PR introduced is gone.

Passthrough default — the 🔴 rethink. Implemented both pieces. translateSQL now throws, naming the statement, rather than handing an untranslated expression to the driver. Only non-literal text is inspected, so SQL-looking text stored as data can't trip it — there's a test for that, since commit_analyses stores commit messages. And mysql-date-expressions.test.ts walks src/lib, extracts every DATE_ADD/DATE_SUB … INTERVAL expression from source, and asserts translateSQL handles it. That's the loop that was missing: a new offender now fails CI instead of a user's request.

Worth noting the guard makes the remaining gaps you listed (INTERVAL n HOUR|MONTH, INTERVAL ? DAY, DATE_ADD(DATE(?), …)) loud rather than silent, which I think is the right trade — I'd rather add a rule when someone needs one than pre-build rules nobody exercises.

Tests. translateSQL is exported, so the rewrite is asserted directly instead of inferred through a driver:

• Dropped the hostile-string test. You were right that it cannot fail — translateSQL never receives params, so interpolation is impossible by signature. Replaced with direct assertions on the rewrite. The PR description's claim about it is wrong and I've stopped making it.
• Boundary tightened from toMatch(/^2026-03-19/) to toBe('2026-03-19 00:00:00').
• Window test kept, inputs corrected to the stored ISO-8601 …T…Z format per your github.ts → report-runner.ts trace. I kept it because a dialect layer is a bad place to undertest, and it's the only case expressing the caller's semantics.
• Sign coverage: '+7 days' and '-7 days' are both pinned, so the copy-paste sign flip you described now fails.
• Temp DB via fs.mkdtempSync with one shared handle, matching cc-apply-breakdowns.test.ts et al., replacing the guessable path and four unclosed handles.

Q2 — why DATE_ADD at all

Fair challenge, and computing the boundary in JS is genuinely cleaner for this query. I didn't take it, deliberately: it fixes one caller and leaves the translator gap for the next one, and the translator is the shared seam. With the guard plus the enforcement test, a future DATE_SUB(?, INTERVAL 7 DAY) now fails in CI rather than reproducing this incident. Happy to do the org.ts change too if you'd prefer belt-and-braces — it's a small follow-up, not a blocker either way.

Deferred, tracked

• committed_at format mismatch — real, and I confirmed your point that the current caller is safe only because its boundary is midnight. Not fixed here; worth its own ticket alongside a strftime normalisation.
• CLAUDE.md:54 stale translator inventory — agreed this is plausibly why the gap went unnoticed. Not in this commit; will land with the doc pass.

Verified locally

Against a seeded SQLite DB in mock mode: GET /api/report/<id>/org → 200 with 8 developers, 16 model-usage rows, populated spend window. The DATE_SUB callers (/api/projects/untracked, /api/projects) → 200. untracked returns empty, which I checked rather than assumed: seeded commits span 2026-03-18–2026-03-31 against a current date of 2026-08-18, so nothing falls inside any 90-day window — unrelated to the emission change.

_{🤖 Generated with Claude Code}