fix: Validate query timezone against IANA zones

[ovr]

The query timezone was only validated as a free-form string, so invalid or malformed values were accepted and handled inconsistently between the native (Tesseract) and legacy planners. Validate it against known IANA zone names so bad values are rejected early with a clear error.

[claude]

Claude finished @ovr's task in 4m 49s —— View job

---

Review complete — approach is sound, and it closes a real SQL injection vector. 1 medium, 4 low (4 inline comments)

Full review — findings, verification notes, and task list

Summary

The fix is well-placed. BaseQuery's constructor guard sits upstream of both buildSqlAndParamsRust and the legacy path, so it covers every query source — including ones that never pass through the API gateway's Joi schema (SQL API session timezone, scheduled refresh, pre-agg jobs). The gateway schema then gives callers an early, cheap rejection. Belt-and-braces in the right order.

Worth stating explicitly in the PR description: this is not only a consistency/UX fix. this.timezone is interpolated raw into SQL string literals across most dialects — PostgresQuery.ts:28 (AT TIME ZONE '${this.timezone}'), ClickHouseQuery.ts:50, SnowflakeQuery.ts:38, BigqueryQuery.ts:45, HiveQuery.ts:30, TrinoQuery.ts:7, VerticaQuery.ts:16, MysqlQuery.ts:49 — so a quote-bearing timezone could previously escape that literal. Framing it as a security fix affects how it gets backported and communicated.

Findings

#	Sev	Location	Issue
1	Medium	query.js:59-64, BaseQuery.js:301	moment.tz.zone() is case-insensitive (zones[name.toLowerCase().replace(/\//g,'_')]), so utc / america/new_york pass validation and then fail in Tesseract (query_tools.rs:61, chrono-tz 0.8 parse::<Tz>() is case-sensitive) while the legacy planner accepts them. The cross-planner divergence this PR targets survives for case variants. Suggested moment.tz.names()-Set membership, or canonicalize via moment.tz.zone(v).name.
2	Low	query.js:61	helpers.error('any.invalid') renders as "timezone" contains an invalid value — doesn't name the value or say an IANA name is expected. Reads worse in the array cases ("timezones[0]" ...). Use helpers.message(...).
3	Low	query.js:425	CUBEJS_DEFAULT_TIMEZONE is folded into query.timezone before querySchema.validate, so a misconfigured env var now surfaces on every query as Invalid query format: ... — pointing the operator at their payload when the fault is server config. Validate the env once at startup, or use a distinct message here. (Couldn't post inline: line not in the diff.)
4	Low	base-query.test.ts:127-151	The list is called maliciousTimezones but no entry carries a quote — the actual injection vector is untested. Also: for loop instead of it.each (opaque failures), and no positive-case assertion, so an over-strict validator would still pass.
5	Low	coverage	preAggsJobsRequestSchema.timezones, queryPreAggregationsSchema.timezone(s) and the now-required queryPreAggregationPreviewSchema.timezone all changed with no tests. Separately, scheduledRefreshTimeZones (cubejs-server-core/src/core/optionsValidate.ts:99) and /v1/cubesql's req.body.timezone (gateway.ts:483) remain unvalidated at their entry points — the BaseQuery guard catches them eventually, just not "early with a clear error".

Residual gap noted inline: moment-timezone 0.5.4x ships a newer tzdb than chrono-tz 0.8.2, so recently-added zone names can pass the JS validator and still fail in the native planner. Follow-up material, not a blocker.

Verified, not a problem

• Dependencies — moment-timezone is already a direct dependency of both packages (cubejs-api-gateway ^0.5.46, cubejs-schema-compiler ^0.5.48); no new dep. The moment → moment-timezone import swap in query.js is safe, since moment-timezone re-exports moment (moment.utc, moment.HTML5_FMT at lines 280-281 still resolve).
• Prototype-pollution style bypass — moment.tz.zone('constructor') looks up a plain object literal, but getZone gates on instanceof Zone / typeof === 'string' before returning, so it yields null. No bypass.
• Docs — docs-mintlify/reference/core-data-apis/rest-api/query-format.mdx:45-47 already documents timezone as a TZ Database Name, so validation matches the documented contract. A changelog note that previously-tolerated values (e.g. +05:00) now hard-fail would still help users hitting it.
• Joi usage — .custom(fn, description) signature and schema immutability around timezoneSchema.required() are both correct.
• Perf — moment.tz.zone() per query is a hashed lookup plus a one-time lazy unpack; negligible. (moment.tz.names(), if adopted, should be hoisted to module scope — it builds and sorts a ~600-entry array per call.)

Not verified

Couldn't run yarn test for either package — node_modules is not installed in this checkout and installing the monorepo's dependencies was out of scope for the review. The test changes were reviewed by reading only. CI results should be the source of truth here.

Task list

• Read PR diff
• Check moment-timezone dependency + imports in changed packages
• Verify other timezone entry points (SQL API / Tesseract / pre-agg jobs / scheduled refresh)
• Cross-check JS validation against Rust parse::<Tz>() semantics
• Trace whether timezone reaches SQL text unescaped
• Assess test coverage and error messages
• Check docs for documented timezone contract
• Post inline comments and review

No prior review threads existed on this PR, so nothing to resolve or dedupe against.

· branch [`hanoi`](https://github.com/cube-js/cube/tree/hanoi)

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 59.28%. Comparing base (e77835a) to head (eaf58eb).
⚠️ Report is 2 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (e77835a) and HEAD (eaf58eb). Click for more details.

HEAD has 1 upload less than BASE

Flag	BASE (e77835a)	HEAD (eaf58eb)
cubesql	1	0

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #11575       +/-   ##
===========================================
- Coverage   84.05%   59.28%   -24.77%     
===========================================
  Files         257      228       -29     
  Lines       81582    18197    -63385     
  Branches        0     3678     +3678     
===========================================
- Hits        68576    10789    -57787     
+ Misses      13006     6860     -6146     
- Partials        0      548      +548     

Flag	Coverage Δ
cube-backend	59.28% <100.00%> (?)
cubesql	?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[claude]

Claude finished @ovr's task in 3m 7s —— View job

---

Re-review of 47516ce (normalization commit) — 2 medium, 4 low (3 new inline comments)

Full review — findings, verification notes, and task list

What changed since the last pass

The new commit takes the "normalize" option from the prior review: timezoneSchema now returns zone.name instead of just passing the value through, normalizeQuery canonicalizes at query.js:450, and the tests grew case-variant coverage on both sides. The direction is right — but the normalization only lands on one of the four routes, and BaseQuery still validates without canonicalizing, so the cross-planner divergence the PR targets is not actually closed yet.

Findings

#	Sev	Location	Issue
1	Medium	query.js:513-514, query.js:525, query.js:552, gateway.ts:988	Joi returns the coerced value in value, not by mutating the input. Three of the four call sites destructure only error, so timezoneSchema's return zone.name is dead code there — the pre-agg and jobs routes forward the raw string. Only normalizeQuery is normalized, and only because it redoes the work by hand at line 450.
2	Medium	BaseQuery.js:301, base-query.test.ts:152-176	BaseQuery validates but doesn't canonicalize, and the new positive test now asserts 'utc' / 'america/new_york' construct fine. For any path that skips querySchema (SQL API req.body.timezone at gateway.ts:483, scheduled refresh, and the pre-agg routes from #1) those values reach Tesseract and fail at parse::<Tz>() (query_tools.rs:61, chrono-tz 0.8.2 is case-sensitive) while the legacy planner accepts them. The test enshrines the divergence rather than closing it.
3	Low	query.js:59-67	Normalizing rewrites deprecated aliases to canonical names (Europe/Kiev→Europe/Kyiv, Asia/Calcutta→Asia/Kolkata, US/Eastern→America/New_York). Since timezone feeds pre-aggregation partition/version keys, affected tenants get a pre-agg rebuild on upgrade. Fine as a decision — needs to be a documented behavior change, not a surprise.
4	Low	query.js:61	helpers.error('any.invalid') still renders as "timezone" contains an invalid value — names neither the offending value nor the expected format. Worse in the array cases ("timezones[0]" ...).
5	Low	query.js:428	CUBEJS_DEFAULT_TIMEZONE is folded into query.timezone before querySchema.validate, so a misconfigured env var now surfaces on every query as Invalid query format: ..., pointing the operator at their payload when the fault is server config. Validate the env once at startup, or use a distinct message. (Line not in the diff, so not postable inline.)
6	Low	base-query.test.ts	Still no quote-carrying entry in the negative list, so the injection vector into AT TIME ZONE '${this.timezone}' has no regression test; still a for loop rather than it.each, so a failure doesn't say which input broke.

The cheapest fix for #1 + #2 together is to drop the normalization from Joi (validate only) and canonicalize once in the BaseQuery constructor — this.timezone = zone.name. That's the single choke point every path already passes through, and it makes query.js:450 unnecessary.

Verified, not a problem

• Security fix still intact — this.timezone is interpolated raw into SQL string literals across most dialects (PostgresQuery.ts:28, ClickHouseQuery.ts:50, SnowflakeQuery.ts:38, BigqueryQuery.ts:45, HiveQuery.ts:30, TrinoQuery.ts:7, VerticaQuery.ts:16, MysqlQuery.ts:49). The BaseQuery guard closes that on every path regardless of the normalization gaps above. Worth stating in the PR description/changelog — it affects how this gets backported.
• Dependencies / imports — moment-timezone is already a direct dep of both packages; BaseQuery.js:11 already imported it, and the moment → moment-timezone swap in query.js:2 is safe (moment-timezone re-exports moment, so moment.utc / moment.HTML5_FMT still resolve).
• moment.tz.zone(...)?.name at line 450 — optional chaining is already used elsewhere in this file and the package's babel config handles it.
• timezoneSchema.required() — Joi schemas are immutable, so deriving the required variant for queryPreAggregationPreviewSchema doesn't leak required into the shared schema.
• Perf — one hashed zone lookup per query plus a one-time lazy unpack; negligible.
• Docs — docs-mintlify/reference/core-data-apis/rest-api/query-format.mdx:45-47 documents timezone as a TZ Database Name, so validation matches the documented contract.

Not verified

• Could not execute node or yarn test in this checkout (dependencies not installed / command not permitted), so the moment-timezone alias behavior in finding ResultSet pivot methods documentation  #3 comes from reading addLink/getZone rather than an empirical check — worth a 10-second node -e confirmation before writing it into release notes. The test changes were reviewed by reading only; CI is the source of truth for pass/fail.

Task list

• Diff origin/master...HEAD and identify what the new commit changed
• Trace whether Joi's normalized value actually reaches consumers on each route
• Re-check BaseQuery guard against the Rust parse::<Tz>() semantics (chrono-tz 0.8.2)
• Assess the pre-agg cache-key implications of canonicalization
• Re-assess test coverage against the new behavior
• Dedupe against prior threads; resolve stale ones
• Post inline comments and summary

Resolved one of my own now-addressed threads (the query.js:67 "canonicalize instead of existence-check" comment — the fix direction was taken). Left the BaseQuery.js:303 and base-query.test.ts:176 threads unresolved: both concerns still apply, and the new inline comments extend them with what the latest commit changed. No human-authored threads touched.

· branch [`hanoi`](https://github.com/cube-js/cube/tree/hanoi)

[claude]

Re-review of 6141a98 — most prior findings addressed; 2 medium, 3 low remain (2 new inline comments)

Full review — findings, verification notes, and task list

What changed since the last pass

This commit takes essentially every structural suggestion from the previous round:

• All four Joi call sites now consume value instead of the raw input (query.js:456, :556, :584, gateway.ts:991/1000), so the timezoneSchema normalization actually reaches consumers. The hand-rolled moment.tz.zone(...)?.name at the old line 450 is gone.
• /v1/cubesql gets a real guard via the new exported normalizeTimezone helper (gateway.ts:484) — throws UserError inside the existing try, so it surfaces as a 400.
• scheduledRefreshTimeZones is validated (optionsValidate.ts:109) with a new unit test file.
• base-query.test.ts converted to it.each, and there's now a positive assertion on the rendered SQL (AT TIME ZONE 'America/New_York').
• BaseQuery's message aligned with the Rust side (Incorrect timezone ${tz}).

Coverage is now genuinely good — normalizeQueryPreAggregations, normalizeQueryPreAggregationPreview, normalizeTimezone and optionsValidate all have tests, including the empty-value passthrough cases.

Findings

#	Sev	Location	Issue
1	Medium	query.js:72	helpers.message() is called with a bare string; joi expects a messages object keyed by error code. The sibling validator added in this same PR uses the correct form (optionsValidate.ts:9), as does the existing CubeValidator.ts:392 — this is the only bare-string call in the repo. joi resolves messages['custom'] to undefined and falls back to its "error code is not defined" placeholder, so the intended message is likely never shown. No test asserts the message body on this route (only /Invalid query format/), so CI won't catch it. Also, both validators interpolate the user value into the template source — timezone: "{{#label}}" echoes the label and an unbalanced { can throw during error rendering (400 → 500); pass it as a local ({ tz: value }) instead.
2	Medium	optionsValidate.ts:9, OptsHandler.ts:92/:417	optionsValidate(opts) runs on user-supplied options only, while the env default is merged after — so CUBEJS_SCHEDULED_REFRESH_TIMEZONES (env.ts:273, plain split(',')) bypasses the new schema entirely. The Joi.func() alternative is likewise unchecked (server.ts:797). And the custom validates without canonicalizing (and optionsValidate discards Joi's value), so ['utc'] passes and is forwarded verbatim → RefreshScheduler → BaseQuery → Tesseract parse::<Tz>() failure while the legacy planner accepts it. Scheduled refresh is now the one route where the divergence this PR targets still exists.
3	Low	BaseQuery.js:301 (existing thread)	BaseQuery still validates without canonicalizing, and base-query.test.ts:150-160 now asserts 'utc'/'america/new_york' construct fine on the strength of a comment saying normalization "happens at the API gateway input layer". True for the gateway routes after this commit — not for scheduled refresh (#2) or direct BaseQuery consumers. this.timezone = zone.name in the constructor makes the assertion true and collapses #2's second half at the same time.
4	Low	changelog / docs (existing thread)	Canonicalization rewrites deprecated aliases (Europe/Kiev→Europe/Kyiv, Asia/Calcutta→Asia/Kolkata, US/Eastern→America/New_York). Since timezone feeds pre-agg partition/version keys, affected tenants get a rebuild on upgrade. Fine as a decision — should be an explicit release-note line. Same for previously-tolerated values like +05:00 now hard-failing.
5	Low	base-query.test.ts	Negative list is 'Not/AZone', '+05:00', '+05', '05', 'foo/bar' — still no quote-bearing entry, so the actual injection vector into AT TIME ZONE '${this.timezone}' (PostgresQuery.ts:28) has no regression test. One entry like "UTC' AT TIME ZONE 'UTC" would pin it.

Verified, not a problem

• value vs raw at the new call sites — preAggsJobsRequestSchema and the pre-agg schemas are plain Joi.object().keys(...) without stripUnknown/unknown(true), so unknown keys error rather than being silently dropped; switching to value.selector / value.timezones doesn't lose fields.
• normalizeTimezone on /v1/cubesql — throws inside the existing try, so it routes through handleError and becomes a 400 rather than an unhandled rejection. Empty/undefined passthrough is correct and tested (the SQL API supplies its own default downstream).
• normalizeQueryPreAggregationPreview — { ...query, timezone: value.timezone } is safe; timezone is required() on that schema so value.timezone is always populated.
• New moment-timezone dep in cubejs-server-core — ^0.5.46 already exists as a resolved range in the workspace (cubejs-api-gateway uses the identical range), so no lockfile churn. Ranges differ across packages (0.5.46/0.5.47/0.5.48) but that predates this PR.
• Security fix intact — this.timezone is still interpolated raw into SQL string literals (PostgresQuery.ts:28, ClickHouseQuery.ts:50, SnowflakeQuery.ts:38, BigqueryQuery.ts:45, HiveQuery.ts:30, TrinoQuery.ts:7, VerticaQuery.ts:16, MysqlQuery.ts:49); the BaseQuery guard closes that on every path. Still worth stating in the PR description — it affects backport handling.
• tzdb skew — moment-timezone here is much newer than chrono-tz 0.8.2 (cubesqlplanner/Cargo.toml:20), so recently-added zone names pass JS validation and fail in the native planner. Follow-up, not a blocker.

Not verified

node_modules is not installed in this checkout and I couldn't install joi, so finding #1 rests on reading joi's error-template resolution plus the repo's own precedent (CubeValidator.ts:392, optionsValidate.ts:9) rather than an empirical run. A 10-second check — assert the thrown message in normalize-query.test.ts — settles it either way, and is worth having as a test regardless. Test suites were reviewed by reading only; CI is the source of truth for pass/fail.

Thread housekeeping

Resolved two of my own now-addressed threads: the query.js:540 "normalized value discarded at three call sites" thread (all four sites now use value) and the base-query.test.ts "for-loop / no positive assertion" thread (converted to it.each, SQL assertion added). Left unresolved: BaseQuery.js:303 (canonicalization, finding #3), the query.js alias/changelog thread (finding #4), and the base-query.test.ts positive-block thread (findings #3/#5) — all still apply. No human-authored threads touched.

Task list

• Diff 47516ce..HEAD and identify what the new commit changed
• Re-verify each prior finding against the current code
• Trace the env-var / function paths for scheduledRefreshTimeZones
• Check value vs raw semantics at the switched call sites (unknown-key stripping)
• Check helpers.message usage against repo precedent
• Verify the new moment-timezone dependency needs no lockfile change
• Resolve stale threads, dedupe new comments
• Post inline comments and summary

View job run · branch hanoi

[claude]

Re-review of 5f5d081 — the helpers.message fix is correct; 1 medium, 4 low remain (2 new inline comments)

Full review — findings, verification notes, and task list

What changed since the last pass

One-line commit, exactly the suggested form:

return helpers.message({ custom: '{{#label}} must be a valid IANA time zone, got "{{#tz}}"' }, { tz: value });

That resolves both halves of the previous medium — the messages object is now keyed by error code (so joi resolves messages.custom instead of falling through to its "error code is not defined" placeholder), and the user value moves out of the template source into a local, so a value containing joi template syntax can no longer be re-parsed as a template.

Findings

#	Sev	Location	Issue
1	Medium	optionsValidate.ts:9, OptsHandler.ts:92/:417	Existing thread, still open. optionsValidate(opts) sees user-supplied options only; scheduledRefreshTimeZones: getEnv('scheduledRefreshTimezones') is merged as a default after, so CUBEJS_SCHEDULED_REFRESH_TIMEZONES (env.ts:273, plain split(',')) bypasses the new schema — as does the Joi.func() alternative (server.ts:797). And the custom validates without canonicalizing while optionsValidate discards joi's value, so ['utc'] is forwarded verbatim → RefreshScheduler → BaseQuery → Tesseract parse::<Tz>() failure, legacy planner accepts. Scheduled refresh remains the one route where the divergence this PR targets survives.
2	Low	optionsValidate.ts:9 (new)	The { tz: value } treatment landed in query.js but not in the sibling validator, which still interpolates ${value} into the template source. Same message-fidelity / template-parse exposure the query.js fix just closed.
3	Low	BaseQuery.js:301 (existing thread)	Still validates without canonicalizing, and base-query.test.ts:150-161 asserts 'utc'/'america/new_york' construct fine on the strength of a comment saying normalization happens "at the API gateway input layer" — true for the gateway routes after 6141a98, not for scheduled refresh (#1) or direct BaseQuery consumers. this.timezone = zone.name makes the assertion true and collapses half of #1 at the same time. (The message-colon nit in that thread is fixed — it now reads Incorrect timezone ${tz}, matching the Rust side.)
4	Low	query.js:455 (new)	CUBEJS_DEFAULT_TIMEZONE is folded into query.timezone before querySchema.validate, so a misconfigured env var now surfaces on every request as Invalid query format: "timezone" must be a valid IANA time zone… — accurate about what is wrong, misleading about whose input caused it. normalizeTimezone at startup would fail fast instead.
5	Low	changelog + base-query.test.ts (existing threads)	(a) Canonicalization rewrites deprecated aliases (Europe/Kiev→Europe/Kyiv, Asia/Calcutta→Asia/Kolkata, US/Eastern→America/New_York); since timezone feeds pre-agg partition/version keys, affected tenants get a rebuild on upgrade — needs an explicit release-note line, along with previously-tolerated values like +05:00 now hard-failing. (b) The negative list still has no quote-bearing entry, so the injection vector into AT TIME ZONE '${this.timezone}' (PostgresQuery.ts:28) has no regression test; one case like "UTC' AT TIME ZONE 'UTC" would pin it.

Verified, not a problem

• helpers.message(messages, local) — two-argument form is the documented joi 17 signature for custom methods, and {{#tz}} is the correct local-context reference (# prefix). Matches the repo's own precedent at CubeValidator.ts:392.
• All four joi call sites consume value — query.js:474, :559, :585, gateway.ts:1000. None of these schemas use stripUnknown/unknown(true), so unknown keys error rather than being silently dropped; switching from raw to value loses nothing. normalizeQueryPreAggregationPreview's { ...query, timezone: value.timezone } is safe because that field is required().
• Security fix intact — this.timezone still reaches SQL string literals raw across dialects (PostgresQuery.ts:28, ClickHouseQuery.ts:50, SnowflakeQuery.ts:38, BigqueryQuery.ts:45, HiveQuery.ts:30, TrinoQuery.ts:7, VerticaQuery.ts:16, MysqlQuery.ts:49); the BaseQuery guard closes that on every path. Still worth stating in the PR description — it affects how this gets backported.
• Coverage — normalizeQuery, normalizeQueryPreAggregations, normalizeQueryPreAggregationPreview, normalizeTimezone and optionsValidate all have tests including empty-value passthrough, and base-query.test.ts:163 now asserts the rendered AT TIME ZONE 'America/New_York'. One gap: the joi-route tests assert only /Invalid query format/, never the rendered message body, so the fix in this commit has no direct regression test — the only assertion on the text (normalize-query.test.ts:105) covers the non-joi normalizeTimezone path.
• tzdb skew — moment-timezone is much newer than chrono-tz 0.8.2 (cubesqlplanner/Cargo.toml:20), so recently-added zone names pass JS validation and fail in the native planner. Follow-up, not a blocker.

Not verified

node_modules is not installed here and running node wasn't permitted, so the joi template-resolution reasoning in #2 and the alias list in #5(a) come from reading joi's template compiler and moment-timezone's addLink/getZone rather than an empirical check. Both are worth a 10-second node -e confirmation before the alias behavior goes into release notes. Test suites were reviewed by reading only; CI is the source of truth for pass/fail.

Thread housekeeping

Nothing to resolve this round — all four unresolved claude threads still have live concerns (BaseQuery.js:303, the query.js alias/changelog thread, the base-query.test.ts positive-block thread, and optionsValidate.ts:9). Re-affirmed rather than re-posted; the unresolved state already communicates that. The bare-string helpers.message thread was addressed by this commit and is already resolved. No human-authored threads touched.

Task list

• Diff 6141a98..HEAD and identify what changed
• Verify the helpers.message({ custom }, { tz }) form against joi's API and repo precedent
• Re-check each prior finding against current code
• Confirm all joi call sites consume the normalized value
• Check test coverage for the message body specifically
• List review threads, dedupe, resolve stale ones (none stale)
• Post inline comments and summary

View job run · branch hanoi

[mintlify]

Preview deployment for your docs. Learn more about Mintlify Previews.

Project	Status	Preview	Updated (UTC)
cubed3	🟢 Ready	View Preview	Aug 17, 2026, 3:33 PM

💡 Tip: Enable Workflows to automatically generate PRs for you.

[claude]

Claude Code is working…

I'll analyze this and get back to you.

View job run

[claude]

Claude finished @ovr's task in 5m 38s —— View job

---

Re-review of 654610b..eaf58eb — env vars now fail fast at boot; 1 medium, 4 low. I was wrong about alias canonicalization — retracted below.

Full review — findings, retraction, verification notes, and task list

What changed since 5f5d081

Three commits, and they move the validation to the right layer:

• canonicalTimezone extracted to @cubejs-backend/shared (timezone.ts) — one implementation now shared by the gateway schema, optionsValidate, and BaseQuery. It guards typeof value !== 'string', so a non-string can no longer reach moment.tz.zone() and turn a 400 into a 500. Good call, and the comment says why.
• Env vars validated eagerly — CUBEJS_DEFAULT_TIMEZONE and CUBEJS_SCHEDULED_REFRESH_TIMEZONES both throw InvalidConfiguration at read time (env.ts:355, :273), and OptsHandler.assertOptions probes defaultTimezone so the failure lands at boot rather than on every request. scheduledRefreshTimezones is read eagerly in initializeCoreOptions (OptsHandler.ts:423), which the comment correctly notes needs no separate probe. This closes the two entry-point gaps I flagged in the last two passes.
• optionsValidate now uses the shared helper with the { tz: value } local, matching query.js:63.
• moment-timezone dropped from cubejs-server-core — verified no remaining usage in that package's src/test. It's already a direct dep of cubejs-backend-shared (^0.5.47) and used by time.ts, so the new import adds no dependency and no extra tz-data load.
• Test coverage is now solid: timezone.test.ts, the defaultTimezone/scheduledRefreshTimezones env blocks, the OptsHandler boot-failure tests, the message-body assertion in optionsValidate.test.ts, and the CUBEJS_DEFAULT_TIMEZONE fallback tests in normalize-query.test.ts.

Retraction

My earlier claim that normalizing to zone.name rewrites deprecated aliases (Europe/Kiev→Europe/Kyiv, US/Eastern→America/New_York) and would churn pre-agg partition keys was wrong. moment-timezone's getZone sets zone.name = names[name] for a link — the requested link name in its canonical casing, not the target's name. timezone.test.ts:22-28 pins exactly that (US/Pacific → US/Pacific), so no cache keys move and the docs: commit dropping those notes is correct. I've resolved that thread. (Verified by reading getZone/addLink; I couldn't execute node here, so CI on timezone.test.ts is the confirmation.)

Findings

#	Sev	Location	Issue
1	Medium	env.ts:282-295	A trailing comma now fails the boot. CUBEJS_SCHEDULED_REFRESH_TIMEZONES=UTC, splits to ['UTC', ''], canonicalTimezone('') returns null, so the server refuses to start with Value "" is not valid for CUBEJS_SCHEDULED_REFRESH_TIMEZONES… — pointing at an empty string rather than the separator. Same for UTC,,America/Los_Angeles. Trailing commas in comma-separated env vars are common in .env and compose YAML, so this is a realistic upgrade break. Previously the empty segment was passed through as '' (itself broken — falsy, so BaseQuery skipped validation and the refresh ran AT TIME ZONE ''), so failing loudly is an improvement, but .map(t => t.trim()).filter(Boolean) before the check is both closer to the old behavior and to what a trailing comma means. env.test.ts covers only well-formed input plus one invalid zone — a 'UTC,' case would pin it. Fix this →
2	Low	optionsValidate.ts:12, :165	return name is dead code on this route — optionsValidate destructures only error, so joi's coerced value is discarded and the canonical name never reaches the caller. Either consume value and return it from optionsValidate, or return value here and drop the canonicalization, so the code doesn't imply a normalization it doesn't do.
3	Low	optionsValidate.ts:109, server.ts:797	The Joi.func() alternative for scheduledRefreshTimeZones (types.ts:221) is still unvalidated — its awaited return value goes straight to queryingOptions.timezones. Combined with BaseQuery deliberately not canonicalizing, a config returning ['utc'] (array or function form) reaches Tesseract's parse::<Tz>() (query_tools.rs:61, case-sensitive) and fails, while the legacy planner accepts it. Much narrower now that the env path is canonical — but it's the last place the divergence this PR targets survives. A canonicalTimezone pass over the resolved list in server.ts closes it.
4	Low	base-query.test.ts:140-147 (existing thread)	Negative list is 'Not/AZone', '+05:00', '+05', '05', 'foo/bar' — still no quote-bearing entry, so the actual injection vector into AT TIME ZONE '${this.timezone}' (PostgresQuery.ts:28) has no regression test. One case like "UTC' AT TIME ZONE 'UTC" would pin the thing this PR is really protecting. timezone.test.ts has the same gap.
5	Low	environment-variables.mdx:1409	CUBEJS_DEFAULT_TIMEZONE got the new <Warning> about failing at startup, but CUBEJS_SCHEDULED_REFRESH_TIMEZONES — which now also fails at startup, and rejects offsets the same way — didn't. Worth the matching note, since that's the one users are most likely to have set to something loose.

Nits, not findings: canonicalTimezone(value: string | null) is typed narrower than it behaves (the tests deliberately pass 123/{}/undefined) — unknown would describe the contract the guard actually implements. And query.js:2 no longer uses moment.tz, so the import could go back to plain moment.

Verified, not a problem

• BaseQuery deliberately not canonicalizing is now a documented decision with a stated rationale (rewriting this.timezone would change generated SQL and pre-agg keys for queryRewrite / refresh-scheduler callers) and an explicit test asserting the behavior. I'd flagged the opposite for three passes; with the env paths now canonical at the source, the remaining exposure is only ResultSet pivot methods documentation  #3, so the tradeoff reads as reasonable. Resolved that thread.
• getEnv('defaultTimezone') per request (query.js:445) — one hashed zone lookup, negligible; and the boot probe means the throw is unreachable in the normal path. Note it's an InvalidConfiguration, not a UserError, so for an embedder using cubejs-api-gateway without server-core a bad env would be a 500 rather than a 400 — acceptable, since it is genuinely a server fault.
• scheduledRefreshTimezones returning undefined rather than null for unset — matches the previous implicit-return behavior and the optional-property type; the comment explains why it matters for Joi.alternatives().
• Non-string handling in BaseQuery — 123/{}/true are truthy, hit canonicalTimezone, get null, and throw UserError; the new it.each covers all three.
• Security fix intact — this.timezone still reaches SQL string literals raw across dialects (PostgresQuery.ts:28, ClickHouseQuery.ts:50, SnowflakeQuery.ts:38, BigqueryQuery.ts:45, HiveQuery.ts:30, TrinoQuery.ts:7, VerticaQuery.ts:16, MysqlQuery.ts:49); the BaseQuery guard closes that on every path. Still worth stating in the PR description — it affects how this gets backported.
• tzdb skew — moment-timezone here is much newer than chrono-tz 0.8.2 (cubesqlplanner/Cargo.toml:20), so recently-added zone names pass JS validation and fail in the native planner. Follow-up, not a blocker.

Not verified

Test execution wasn't permitted in this run (yarn jest blocked), and node -e likewise, so findings #1 and the retraction rest on reading env-var, joi, and moment-timezone's getZone/addLink rather than an empirical run. CI is the source of truth for pass/fail — in particular timezone.test.ts's link-name assertions are what confirm the retraction.

Inline comments could not be posted this pass — the GitHub review-comment API returned 503 No server is currently available on four attempts, so findings #1 and #2/#3 are written out above instead of on their lines.

Thread housekeeping

Resolved four of my own now-stale threads: BaseQuery.js case-canonicalization (addressed by an explicit documented decision + test), query.js:67 alias-rewrite (my claim was wrong — retracted above), optionsValidate.ts env-var bypass (fixed in env.ts + OptsHandler), and query.js:445 startup validation of CUBEJS_DEFAULT_TIMEZONE (fixed). Left the base-query.test.ts thread unresolved — finding #4 is still live in it. No human-authored threads touched.

Task list

• Diff 5f5d081..HEAD and identify what changed
• Re-verify each open finding against the current tree
• Review the new timezone.ts helper + env changes
• Confirm the moment-timezone dep move (added to backend-shared? removed from server-core?)
• Re-check the alias/link canonicalization claim against moment-timezone's getZone
• Check test coverage of new code
• List review threads, resolve stale ones, dedupe
• Post summary (inline comments blocked by API 503s)

· branch [`hanoi`](https://github.com/cube-js/cube/tree/hanoi)

[claude]

Claude finished @ovr's task in 2m 43s —— View job

---

Re-review of 062b0e7 — the trailing-comma break is fixed. 0 medium, 3 low remain (1 new inline comment)

Full review — findings, verification notes, and task list

What changed since eaf58eb

One commit, and it takes the medium from the last pass:

const timezones = get('CUBEJS_SCHEDULED_REFRESH_TIMEZONES')
  .default('')
  .asArray()
  .map(timezone => timezone.trim())
  .filter(Boolean);

asArray() already drops empty segments, the .trim()/.filter(Boolean) pair then handles whitespace-only entries, and the .default('') is what keeps the unset case from calling .map on undefined (env-var returns undefined, not [], for an unset var with no default). env.test.ts now pins 'UTC,', 'UTC,,America/Los_Angeles', ' UTC , ', '', ' ' and ',' — exactly the cases I flagged, plus the blank ones.

The undefined → [] change is safe — I traced every consumer of the option:

• OptsHandler.ts:423 puts it in the defaults object before ...opts, so a user-supplied value still wins whether the default is [] or undefined.
• server.ts:202 is the only reader: wrapToFnIfNeeded(this.options.scheduledRefreshTimeZones || []) — [] and undefined both land on [].
• server.ts:797 gates on timezonesFromOptionsOrSecurityContext.length > 0, so an empty list means "leave queryingOptions.timezones alone", same as before.
• optionsValidate only ever sees user options, so the [] default never reaches Joi.alternatives() — which is what the now-removed comment was guarding against. Removing it is correct, though the Joi.array() branch would have accepted [] anyway.

Nothing else in packages/*/src reads getEnv('scheduledRefreshTimezones').

Findings

#	Sev	Location	Issue
1	Low	optionsValidate.ts:11 (new inline)	return name is dead code — the default export destructures only error (:164), so joi's coerced value is discarded and the canonical name never reaches the caller. Now that the env var canonicalizes at the source, the only remaining ways in are the config-file array and the Joi.func() alternative (:108, resolved at server.ts:797, still unvalidated) — so scheduledRefreshTimeZones: ['utc'] still reaches Tesseract's case-sensitive parse::<Tz>() while the legacy planner accepts it. Narrow, but the return name implies a normalization that never lands.
2	Low	environment-variables.mdx:1409-1419	CUBEJS_DEFAULT_TIMEZONE got a <Warning> about failing at startup (:1109); CUBEJS_SCHEDULED_REFRESH_TIMEZONES — which now also fails the boot on an invalid zone, and also rejects +05:00 — still has none. It's the var most likely to be set loosely, and this commit adds a second documentable behavior (empty segments are ignored, so trailing commas are fine). Line isn't in the diff, so not postable inline. Fix this →
3	Low	base-query.test.ts:140-147, timezone.test.ts:29-42	Neither negative list has a quote-bearing entry, so the injection vector into AT TIME ZONE '${this.timezone}' (PostgresQuery.ts:28) still has no regression test. One case like "UTC' AT TIME ZONE 'UTC" would pin the thing this PR is really protecting. (The thread on this was resolved — re-flagging once here since the gap is still open; your call.)

Nit, not a finding: canonicalTimezone(value: string | null) is typed narrower than it behaves — timezone.test.ts:44-57 deliberately passes 123/{}/undefined and the guard handles them, so unknown would describe the actual contract.

Verified, not a problem

• .default('') + .asArray() — env-var's array accessor returns [] for an empty string and .filter(Boolean)s the segments, so ',' → [] and 'UTC,,X' → ['UTC','X'] before the trim pass ever runs. The redundancy is harmless and the trim is what makes ' UTC , ' work.
• Removing the undefined-must-stay-undefined comment — traced above; no consumer distinguishes the two.
• canonicalTimezone link handling — timezone.test.ts:19-27 pins US/Pacific/Asia/Calcutta/Europe/Kiev as returning themselves, which is what makes my earlier (retracted) alias-churn claim moot and the docs: commit dropping those notes correct.
• Security fix intact — this.timezone still reaches SQL string literals raw across dialects (PostgresQuery.ts:28, ClickHouseQuery.ts:50, SnowflakeQuery.ts:38, BigqueryQuery.ts:45, HiveQuery.ts:30, TrinoQuery.ts:7, VerticaQuery.ts:16, MysqlQuery.ts:49); the BaseQuery constructor guard closes that on every path. Still worth a line in the PR description — it affects how this gets backported.
• tzdb skew — moment-timezone here is much newer than chrono-tz 0.8.2 (cubesqlplanner/Cargo.toml:20), so recently-added zone names pass JS validation and fail in the native planner. Follow-up, not a blocker.

Not verified

node_modules isn't installed in this checkout and I couldn't run the suites or node -e, so the env-var asArray/default semantics above come from reading the accessor rather than an empirical run — the new env.test.ts cases are exactly what confirms them, so CI is the source of truth.

Thread housekeeping

All ten prior claude threads are already resolved, so nothing to resolve this pass and nothing to dedupe against. Posted one new inline comment (finding #1); #2's line isn't in the diff and #3's thread was resolved by the author, so both are written out above instead.

Task list

• Diff eaf58eb..062b0e7 and identify what changed
• Trace every consumer for the undefined → [] behavior change
• Check .default('') / asArray() semantics against the new tests
• Re-verify each open finding against the current tree
• List review threads, resolve stale ones (none open)
• Post inline comment and summary

· branch [`hanoi`](https://github.com/cube-js/cube/tree/hanoi)

[claude]

Claude finished @ovr's task in 3m 2s —— View job

---

Re-review of 12d107d — closes the last open medium/low on the config route. 0 medium, 3 low remain (1 new inline comment)

Full review — findings, verification notes, and task list

What changed since 062b0e7

One commit, taking the remaining finding from the last pass:

• optionsValidate.ts default export → named validateOptions<T>(options: T): T, now returning joi's coerced value instead of discarding it.
• OptsHandler constructor becomes const options = this.assertOptions(cloneDeep(this.createOptions)), and assertOptions returns the sanitized copy — so scheduledRefreshTimeZones: ['utc'] from cube.js reaches initializeCoreOptions as ['UTC'] and flows on to server.ts:202 → RefreshScheduler → BaseQuery. OptsHandler.test.ts pins both the throw and the canonicalization end-to-end (core.options.scheduledRefreshTimeZones → ['UTC', 'America/New_York']).
• New validateOptions tests for function-by-reference preservation and input non-mutation.

Ordering is right: the dbType-was-removed check still runs before validateOptions, so that keeps its bespoke migration message rather than joi's generic "dbType" is not allowed. And this.createOptions isn't read anywhere else in OptsHandler, so nothing downstream sees the pre-canonicalization copy.

The rename is internal-only — nothing outside OptsHandler.ts and the test imported the default export (checked packages/*/src), so no consumer breaks.

Findings

#	Sev	Location	Issue
1	Low	optionsValidate.ts:163-169 (new inline)	Returning joi's value lands every coercion, not just the time zone one. The top level isn't .strict() (only the nested http/jwt/orchestratorOptions/queue objects are), so with convert: true: numeric strings become numbers (scheduledRefreshConcurrency: '4' → 4, sqlPort, gracefulShutdown, …), 'true'/'false' become booleans (telemetry, sqlCache, and the scheduledRefreshTimer alternative), and keys explicitly set to undefined are dropped — so { scheduledRefreshTimeZones: undefined } in cube.js no longer shadows the env default in the ...opts spread. All three read as improvements, but the assertOptions doc comment describes the return as "canonical time zone names", which undersells it, and these are user-visible config semantics changing inside a timezone PR. Widen the comment + release note; a test that a nested strict object round-trips (…queueOptions.sendProcessMessageFn still toBe the same function) would pin the part that could bite.
2	Low	optionsValidate.ts:108, server.ts:202/:797	The Joi.func() alternative for scheduledRefreshTimeZones is still unvalidated — its awaited return value goes straight into queryingOptions.timezones. With the env var canonicalizing at the source and the array form now canonicalizing here, the function form is the last route where the cross-planner divergence survives: a config returning ['utc'] reaches Tesseract's case-sensitive parse::<Tz>() (query_tools.rs:61) and fails, while the legacy planner accepts it. A canonicalTimezone pass over the resolved list at server.ts:797 closes it for good.
3	Low	environment-variables.mdx:1409-1419	Still no <Warning> under CUBEJS_SCHEDULED_REFRESH_TIMEZONES, while CUBEJS_DEFAULT_TIMEZONE got one at :1109. This var now also fails the boot on an invalid zone, also rejects +05:00, and (since 062b0e7) ignores empty entries from trailing/repeated commas — three documentable behaviors, zero doc lines. Fix this →
4	Low	base-query.test.ts:140-147, timezone.test.ts:29-42	Neither negative list has a quote-bearing entry, so the injection vector into AT TIME ZONE '${this.timezone}' (PostgresQuery.ts:28) — the thing this PR is really protecting — has no regression test. One case like "UTC' AT TIME ZONE 'UTC" would pin it. (Author resolved this thread previously; re-flagging once, your call.)

Nit, unchanged: canonicalTimezone(value: string | null) is typed narrower than it behaves — timezone.test.ts:44-57 deliberately passes 123/{}/undefined and the guard handles them, so unknown describes the actual contract.

Verified, not a problem

• cloneDeep moved before validation — lodash preserves function references when they're object properties, so driverFactory/logger/contextTo* survive the clone exactly as before; the new validateOptions test pins the joi half of that.
• Rename blast radius — no import optionsValidate remains anywhere in packages/*/src; not re-exported from cubejs-server-core's public index.
• undefined-key deletion vs. consumers — the only presence-style checks in OptsHandler (:487, :579, :623, :647) all test === undefined / !== undefined, which a deleted key satisfies identically.
• Security fix intact — this.timezone still reaches SQL string literals raw across dialects (PostgresQuery.ts:28, ClickHouseQuery.ts:50, SnowflakeQuery.ts:38, BigqueryQuery.ts:45, HiveQuery.ts:30, TrinoQuery.ts:7, VerticaQuery.ts:16, MysqlQuery.ts:49); the BaseQuery constructor guard closes it on every path. Still worth one line in the PR description — it affects how this gets backported.
• tzdb skew — moment-timezone here is much newer than chrono-tz 0.8.2 (cubesqlplanner/Cargo.toml:20), so recently-added zone names pass JS validation and fail in the native planner. Follow-up, not a blocker.

Not verified

node_modules isn't installed in this checkout and node -e wasn't permitted, so the joi convert/clone semantics in finding #1 (numeric-string coercion, undefined-key deletion, .strict(true) subtrees not converting) come from reading joi 17's object validation rather than an empirical run. The suggested round-trip test is exactly what would settle it. CI is the source of truth for pass/fail.

Thread housekeeping

Resolved the one open thread (optionsValidate.ts:11, dead return name) — this commit addresses it directly. All other prior claude threads were already resolved. Posted one new inline comment (#1); #2's fix site isn't in the diff, #3's line isn't in the diff, and #4's thread was resolved by the author, so those are written out above instead. No human-authored threads touched.

Task list

• Diff 062b0e7..12d107d and identify what changed
• Trace the sanitized-options flow through OptsHandler → server.ts → scheduler
• Check the rename's blast radius across packages
• Assess the wider joi-coercion surface now that value is consumed
• Re-verify each remaining open finding against the current tree
• List review threads, resolve stale ones, dedupe
• Post inline comment and summary

· branch [`hanoi`](https://github.com/cube-js/cube/tree/hanoi)

[claude]

This closes the dead-return name gap from the last pass — the canonical name now actually reaches initializeCoreOptions (OptsHandler.ts:49), and OptsHandler.test.ts pins it. Ordering is right too: the dbType removal check still runs before validateOptions, so that keeps its bespoke message instead of joi's "is not allowed".

One thing worth being deliberate about: returning joi's value doesn't only land the time zone canonicalization — it lands every coercion the schema performs, for the first time. schemaOptions is not .strict() at the top level (only the nested http/jwt/orchestratorOptions/queue objects are), so with joi's default convert: true:

• Joi.number() fields accept and now return numeric strings as numbers — scheduledRefreshConcurrency: '4' → 4, same for gracefulShutdown, dashboardAppPort, sqlPort, compilerCacheSize…
• Joi.boolean() fields turn 'true'/'false' strings into booleans — telemetry, devServer, sqlCache, livePreview, and the scheduledRefreshTimer alternative ('true' now resolves to boolean true rather than staying a string).
• Keys explicitly set to undefined are deleted from the returned object, so { scheduledRefreshTimeZones: undefined } in cube.js no longer shadows the env default in the ...opts spread at OptsHandler.ts:~420.

All three read as improvements to me, and nested objects/functions are unaffected (.strict(true) subtrees don't convert, and Joi.func() values come back by reference — the new test pins that). But the doc comment on assertOptions describes the return as "joi coercions (canonical time zone names)", which undersells it. Worth widening that comment, and worth a release-note line, since these are user-visible config semantics changing inside a PR titled as a timezone validation fix.

Behavior I'd suggest pinning while you're here, since it's the one that could bite: a test asserting a nested strict object round-trips unchanged (e.g. orchestratorOptions.queryCacheOptions.queueOptions.sendProcessMessageFn still toBe the same function after validateOptions).

[claude]

Claude finished @ovr's task in 2m 6s —— View job

---

Re-review of 3a5784b — rename is clean; 0 medium, 4 low (1 new inline comment)

Full review — findings, verification notes, and task list

What changed since 12d107d

Pure rename + comment cleanup: OptsHandler.assertOptions → validateOptions, plus removal of several JSDoc blocks. Behavior is unchanged, and the rename is internally consistent — no assertOptions reference remains anywhere in packages/*/src or the tests, and the method was private, so nothing outside the class could have called it. The name also now matches what the method actually does (validate + return sanitized), which is the point of the rename.

Most of the deleted comments were pure boilerplate (/** Class constructor. */, /** Initialized options. */, /** Driver service class. */ on a class named OptsHandler) — good to see them go. Two of them weren't, though; see #1.

Findings

#	Sev	Location	Issue
1	Low	OptsHandler.ts:63, :73, :76 (new inline)	The private method is now named exactly like the imported function it calls, and line 73 invokes the import from inside a same-named method. Correct (class methods aren't in lexical scope) but reads like unguarded recursion, and adding this. there would silently change which function runs. Separately, two of the removed comments carried real rationale: the getEnv('defaultTimezone') probe lost the half explaining why CUBEJS_SCHEDULED_REFRESH_TIMEZONES needs no probe — a discarded-result getEnv(...) is exactly what gets deleted as dead code later — and the return-value doc is gone entirely, which is the opposite direction from #2.
2	Low	optionsValidate.ts:163-169 (existing thread, still open)	Returning joi's value lands every coercion for the first time, not just the time zone one: the top level isn't .strict(), so numeric strings become numbers (scheduledRefreshConcurrency: '4' → 4, sqlPort, gracefulShutdown…), 'true'/'false' become booleans (telemetry, sqlCache, the scheduledRefreshTimer alternative), and explicitly-undefined keys are dropped so { scheduledRefreshTimeZones: undefined } no longer shadows the env default. All read as improvements, but they're user-visible config semantics changing inside a timezone PR, and the code comment that described the return just got deleted (#1) rather than widened. Still worth a release-note line, and a test that a nested strict subtree round-trips (…queueOptions.sendProcessMessageFn still toBe the same function).
3	Low	optionsValidate.ts:108, server.ts:797	The Joi.func() alternative for scheduledRefreshTimeZones remains unvalidated — its awaited result goes straight into queryingOptions.timezones. With the env var canonicalizing at the source and the array form canonicalizing in validateOptions, the function form is the last route where the cross-planner divergence survives: a config returning ['utc'] fails Tesseract's case-sensitive parse::<Tz>() (query_tools.rs:61) while the legacy planner accepts it. A canonicalTimezone pass over the resolved list at server.ts:797 closes it.
4	Low	environment-variables.mdx:1409-1419	Still no <Warning> under CUBEJS_SCHEDULED_REFRESH_TIMEZONES, while CUBEJS_DEFAULT_TIMEZONE has one at :1109. This var now also fails the boot on an invalid zone, also rejects +05:00, and (since 062b0e7) ignores empty entries from trailing/repeated commas — three documentable behaviors, zero doc lines. Fix this →
5	Low	base-query.test.ts:140-146, timezone.test.ts:29-42	Neither negative list has a quote-bearing entry, so the injection vector into AT TIME ZONE '${this.timezone}' (PostgresQuery.ts:28) — the thing this PR is really protecting — still has no regression test. One case like "UTC' AT TIME ZONE 'UTC" would pin it. (Author resolved this thread earlier; mentioning once more, your call.)

Nit, unchanged: canonicalTimezone(value: string | null) is typed narrower than it behaves — timezone.test.ts:44-57 deliberately passes 123/{}/undefined and the guard handles them, so unknown describes the actual contract.

Verified, not a problem

• Rename blast radius — grep -rn assertOptions packages/ returns nothing; the only validateOptions references are the module export (optionsValidate.ts:163), the import + two uses in OptsHandler.ts, and the test file. Nothing re-exported from cubejs-server-core's public surface.
• Ordering preserved — the dbType-was-removed check still runs before validateOptions(opts), so that keeps its bespoke migration message rather than joi's generic "dbType" is not allowed.
• getEnv('defaultTimezone') probe still present (line 76), so CUBEJS_DEFAULT_TIMEZONE still fails at boot rather than on every request.
• Test coverage carried over — optionsValidate.test.ts (throw, canonicalization, function-by-reference, non-mutation) and OptsHandler.test.ts (boot failure + core.options.scheduledRefreshTimeZones → ['UTC', 'America/New_York']) both still exercise the renamed path through the public constructor.
• Security fix intact — this.timezone still reaches SQL string literals raw across dialects (PostgresQuery.ts:28, ClickHouseQuery.ts:50, SnowflakeQuery.ts:38, BigqueryQuery.ts:45, HiveQuery.ts:30, TrinoQuery.ts:7, VerticaQuery.ts:16, MysqlQuery.ts:49); the BaseQuery constructor guard closes it on every path. Still worth one line in the PR description — it affects how this gets backported.
• tzdb skew — moment-timezone here is much newer than chrono-tz 0.8.2 (cubesqlplanner/Cargo.toml:20), so recently-added zone names pass JS validation and fail in the native planner. Follow-up, not a blocker.

Not verified

node_modules isn't installed in this checkout, so I couldn't run yarn tsc or the suites; the rename was verified by grep across packages/*/src rather than a compile. CI is the source of truth for pass/fail.

Thread housekeeping

One thread was open (optionsValidate.ts:169, joi-coercion breadth) and it still applies — this commit deletes the doc comment that thread asked to be widened, so I left it unresolved and extended the point in the new inline comment. All other prior claude threads were already resolved. Findings #3 and #4 aren't on diff lines, and #5's thread was resolved by the author, so those are written out above instead. No human-authored threads touched.

Task list

• Diff 12d107d..3a5784b and identify what changed
• Check rename blast radius across packages
• Re-verify each remaining open finding against the current tree
• List review threads, resolve stale ones (one open, still applies)
• Post inline comment and summary

· branch [`hanoi`](https://github.com/cube-js/cube/tree/hanoi)

[claude]

Claude finished @ovr's task in 1m 35s —— View job

---

Re-review of c82de0b — name collision resolved. 0 medium, 4 low remain (no new inline comments; 1 thread resolved)

Full review — findings, verification notes, and task list

What changed since 3a5784b

Two-line rename: OptsHandler.validateOptions → sanitizeOptions (OptsHandler.ts:43, :63). Behavior unchanged.

This takes finding #1 from the last pass and picks the better of the two options I suggested: rather than aliasing the import, the method got a name that no longer collides with the imported validateOptions it calls at line 73. It also reads more accurately — the method does more than validate (it rejects dbType, probes getEnv('defaultTimezone'), checks the driver-factory requirement, and returns the sanitized copy), so sanitizeOptions describes it better than either previous name. Grep confirms the rename is complete: the only validateOptions references left are the module export (optionsValidate.ts:163), the import + call in OptsHandler.ts, and optionsValidate.test.ts; no assertOptions anywhere. The method is private, so nothing outside the class could break.

Ordering still correct: the dbType-was-removed check runs before validateOptions(opts), keeping its bespoke migration message instead of joi's generic "dbType" is not allowed.

Findings

#	Sev	Location	Issue
1	Low	optionsValidate.ts:163-169 (existing thread, still open)	Returning joi's value lands every coercion, not just the time zone one. The top level isn't .strict() (only the nested http/jwt/orchestratorOptions/queue objects are), so with convert: true: numeric strings become numbers (scheduledRefreshConcurrency: '4' → 4, sqlPort, gracefulShutdown…), 'true'/'false' become booleans (telemetry, sqlCache, the scheduledRefreshTimer alternative), and explicitly-undefined keys are dropped so { scheduledRefreshTimeZones: undefined } no longer shadows the env default. All read as improvements, but they're user-visible config semantics changing inside a timezone PR, and the doc comment that described the return was deleted in 3a5784b rather than widened — so this is now undocumented both in code and in the release notes. A test that a nested strict subtree round-trips (…queueOptions.sendProcessMessageFn still toBe the same function) would pin the part that could bite.
2	Low	optionsValidate.ts:108, server.ts:797	The Joi.func() alternative for scheduledRefreshTimeZones is still unvalidated — await this.scheduledRefreshTimeZones(context) goes straight into queryingOptions.timezones (server.ts:798). With the env var canonicalizing at the source and the array form canonicalizing in validateOptions, the function form is the last route where the cross-planner divergence survives: a config returning ['utc'] fails Tesseract's case-sensitive parse::<Tz>() (query_tools.rs:61) while the legacy planner accepts it. A canonicalTimezone pass over the resolved list at server.ts:797 closes it.
3	Low	environment-variables.mdx:1409-1419	Still no <Warning> under CUBEJS_SCHEDULED_REFRESH_TIMEZONES, while CUBEJS_DEFAULT_TIMEZONE has one. This var now also fails the boot on an invalid zone, also rejects +05:00, and (since 062b0e7) ignores empty entries from trailing/repeated commas — three documentable behaviors, zero doc lines. Fix this →
4	Low	base-query.test.ts:140-146, timezone.test.ts:29-42	Neither negative list has a quote-bearing entry, so the injection vector into AT TIME ZONE '${this.timezone}' (PostgresQuery.ts:28) — the thing this PR is really protecting — still has no regression test. One case like "UTC' AT TIME ZONE 'UTC" would pin it. (Author resolved this thread earlier; noted once more, your call.)

Residual from the resolved thread, not worth its own row: the getEnv('defaultTimezone') comment at OptsHandler.ts:75 still explains only why the probe exists, not why CUBEJS_SCHEDULED_REFRESH_TIMEZONES needs no sibling probe (initializeCoreOptions reads it eagerly). A discarded-result getEnv(...) is exactly the line someone deletes as dead code later.

Nit, unchanged: canonicalTimezone(value: string | null) is typed narrower than it behaves — timezone.test.ts:44-57 deliberately passes 123/{}/undefined and the guard handles them, so unknown describes the actual contract.

Verified, not a problem

• Rename blast radius — grep -rn 'assertOptions|validateOptions|sanitizeOptions' packages/*/src packages/*/test returns only the export, the import + two call sites in OptsHandler.ts, and the test file. Nothing re-exported from cubejs-server-core's public surface; the method was private.
• opts vs validated inside sanitizeOptions — the driver-factory requirement check at line 76-86 still reads opts.driverFactory, but joi returns Joi.func() values by reference and only drops explicitly-undefined keys, so the truthiness test is identical on either object. getDriverFactory / initializeCoreOptions both receive the sanitized copy.
• Test coverage carried over — optionsValidate.test.ts (throw, canonicalization, function-by-reference, non-mutation) and OptsHandler.test.ts (boot failure + core.options.scheduledRefreshTimeZones → ['UTC', 'America/New_York']) both still exercise the renamed path through the public constructor.
• Security fix intact — this.timezone still reaches SQL string literals raw across dialects (PostgresQuery.ts:28, ClickHouseQuery.ts:50, SnowflakeQuery.ts:38, BigqueryQuery.ts:45, HiveQuery.ts:30, TrinoQuery.ts:7, VerticaQuery.ts:16, MysqlQuery.ts:49); the BaseQuery constructor guard closes it on every path. Still worth one line in the PR description — it affects how this gets backported.
• tzdb skew — moment-timezone here is much newer than chrono-tz 0.8.2 (cubesqlplanner/Cargo.toml:20), so recently-added zone names pass JS validation and fail in the native planner. Follow-up, not a blocker.

Not verified

node_modules isn't installed in this checkout, so I couldn't run yarn tsc or the suites; the rename was verified by grep across packages/*/src and packages/*/test rather than a compile. The joi convert semantics in finding #1 come from reading joi 17's object validation. CI is the source of truth for pass/fail.

Thread housekeeping

Resolved the OptsHandler.ts naming thread — this commit addresses its headline concern directly (and the diff it was anchored to is outdated); its residual comment point is carried above instead. Left optionsValidate.ts:169 open — finding #1 still applies, and this commit didn't touch it. Findings #2 and #3 aren't on diff lines and #4's thread was resolved by the author, so all three are written out above rather than posted inline. No new inline comments this pass — nothing in c82de0b introduces a new issue. No human-authored threads touched.

Task list

• Diff 3a5784b..HEAD and identify what changed
• Check rename blast radius across packages
• Verify opts vs validated usage inside the renamed method
• Re-verify each remaining open finding against the current tree
• List review threads, resolve stale ones, dedupe
• Post summary

· branch [`hanoi`](https://github.com/cube-js/cube/tree/hanoi)