Skip to content

refactor(api): make a validated warehouse timestamp its own type - #630

Merged
Makisuo merged 3 commits into
mainfrom
worktree-fix-warehouse-datetime-params
Aug 25, 2026
Merged

refactor(api): make a validated warehouse timestamp its own type#630
Makisuo merged 3 commits into
mainfrom
worktree-fix-warehouse-datetime-params

Conversation

@Makisuo

@Makisuo Makisuo commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Follow-up to #627. That PR fixed the two malformed-DateTime bugs, but it fixed them with hand-rolled checks and an invalid array each tool had to remember to inspect — a guard that works until someone adds tool 58 and doesn't. This replaces that with a type.

A validated timestamp becomes its own type

packages/query-engine/src/datetime.ts gains two schemas:

  • WarehouseDateTime — branded, canonical YYYY-MM-DD HH:mm:ss UTC. Safe to interpolate into a DateTime comparison. It cannot be produced by string manipulation, only by decoding or by warehouseDateTime(epochMs).
  • WarehouseTimeInput — decodes every encoding a caller plausibly sends (warehouse shape, ISO-8601 with Z or an offset, fractional seconds, a bare date) into the canonical form.

Two things the validation does that a NaN check doesn't, both found by writing the tests:

  • Grammar is checked before parseability. Date.parse("2026") succeeds, as does Date.parse("Aug 25 2026"). A NaN check alone lets a sheared timestamp through as a confidently wrong window.
  • Calendar fields are checked as written. I assumed V8 rejected 2026-02-30 — it doesn't, it rolls over to 2 March. Month 13 is rejected, day overflow is not. Left to the parser a typo becomes silently wrong data, so the fields are validated directly (leap years included).

Validation moves to the parameter boundary

MCP tools now declare bounds with optionalTimeParam instead of optionalStringParam, so a malformed value fails at decode — before any tool body runs — with a message naming the offending value and the accepted shapes. This is the pattern optionalNumberParam already established, for the same caller: its doc comment cites investigation.hypothesis, which is the agent that sent 2026-08-47:53.

The default rendering for a failed pattern check is the raw regex source, which tells a model nothing about what to send instead, so the check builds its own message.

The type makes it unskippable

resolveTimeRange now accepts only WarehouseDateTime. A tool that declares a time bound as a plain string doesn't get a subtly wrong window at runtime — it fails to compile. The typechecker found the one call site I'd missed (inspect-chart-data), which is the whole argument for doing it this way.

That let #627's scaffolding be deleted outright: normalizeTime, the invalid array, InvalidTimeBound, timeRangeInvalidMessage, timeRangeInvalidResult, and the ten per-tool guards. resolveTimeRange is now total — it has no parsing to do and no malformed case to handle.

One latent bug fixed in passing

The v2 parseWindow tested Number.isNaN(startMs) || Number.isNaN(endMs) || endMs <= startMs and reported all three as "end_time must be later than start_time." — so an unparseable start_time blamed end_time. The NaN branch is also unreachable: every call site passes a value the Timestamp schema already validated at the HTTP boundary. Now it checks ordering only.

Deliberately not changed

listMetricsQuery still compares $.Hour.lte(param.dateTime("endTime")) without a toDateTime() wrapper, asymmetric with the .gte beside it. Wrapping it would be belt-and-braces: the brand makes a fractional value unrepresentable at that call site, which is strictly stronger than defending against it in SQL, and the change would churn the SQL catalog baseline for no behaviour difference.

Testing

  • apps/api MCP + v2 routes: 539 passed
  • packages/query-engine: 1269 passed
  • New datetime-schema.test.ts: 10 cases covering the accepted encodings, offset conversion, 2026-08-47:53, calendar impossibilities, leap years on both sides, and the partials Date.parse would accept
  • time.test.ts rewritten to drive bounds through optionalTimeParam, so the parameter schema and the resolver are exercised on the same path production uses — plus a test pinning the published JSON schema to a plain string, since this file's history is one of parameters publishing as something the decoder then rejects

View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

Two production error classes, one shape: a time string that isn't a valid
warehouse DateTime is spliced into SQL and only fails at ClickHouse.

MCP tools — normalizeTime returned unparseable input verbatim, so an
agent-supplied start_time of '2026-08-47:53' became
toDateTime('2026-08-47:53') and surfaced as an unhandled McpQueryError /
WarehouseMalformedQueryError. It now returns null; resolveTimeRange reports the
bad bound in `invalid` and substitutes the default window's bound, so st/et are
always well-formed even for a caller that doesn't check. The ten tools that
already guard `exceeded` now also return timeRangeInvalidResult, which names the
bad value and the accepted formats so the agent can retry.

v2 telemetry — parseWindow's `precision` defaulted to "millisecond", but
v2ListMetrics reads metric_catalog.Hour, a plain DateTime. Every call 500'd with
'Cannot parse string ...000 as DateTime' and a matching TYPE_MISMATCH. The
handler now asks for second precision, and `precision` is required so the next
rollup-backed handler can't inherit the unsafe default by omission.
Follow-up to #627, which fixed the two malformed-DateTime bugs but left the
validation as hand-rolled checks each tool had to remember to run.

Adds `WarehouseDateTime` — a branded Effect Schema for the canonical
`YYYY-MM-DD HH:mm:ss` warehouse literal — and `WarehouseTimeInput`, which
decodes every encoding a caller plausibly sends (warehouse shape, ISO-8601 with
Z or an offset, fractional seconds, bare date) into it. The grammar is checked
before parseability because `Date.parse` accepts "2026" and other partials, and
the calendar fields are checked as written because V8 rolls 30 Feb over to
2 March rather than rejecting it — either would turn a typo into a confidently
wrong window instead of an error.

MCP tools now declare time bounds with `optionalTimeParam`, so a malformed bound
fails at the parameter decode boundary with a message naming the offending
value, the same pattern `optionalNumberParam` already established for numbers.
`resolveTimeRange` accepts only the brand, so a tool that declares a bound as a
plain string no longer produces a subtly wrong window at runtime — it fails to
compile. That let #627's `normalizeTime`, `invalid` array and per-tool
`timeRangeInvalidResult` guards be deleted outright.

Also drops the unreachable NaN branch in the v2 `parseWindow`, which reported an
unparseable `start_time` as "end_time must be later than start_time" — blaming
the wrong parameter for a case the `Timestamp` schema already rejects upstream.
…e-datetime-params

# Conflicts:
#	apps/api/src/mcp/lib/time.test.ts
#	apps/api/src/mcp/lib/time.ts
#	apps/api/src/mcp/tools/explore-attributes.ts
#	apps/api/src/mcp/tools/find-slow-traces.ts
#	apps/api/src/mcp/tools/get-service-top-operations.ts
#	apps/api/src/mcp/tools/list-metrics.ts
#	apps/api/src/mcp/tools/list-product-events.ts
#	apps/api/src/mcp/tools/mine-log-patterns.ts
#	apps/api/src/mcp/tools/query-funnel.ts
#	apps/api/src/mcp/tools/search-logs.ts
#	apps/api/src/mcp/tools/search-sessions.ts
#	apps/api/src/mcp/tools/search-traces.ts
@Makisuo
Makisuo merged commit 42b58b7 into main Aug 25, 2026
34 checks passed
@Makisuo
Makisuo deleted the worktree-fix-warehouse-datetime-params branch August 25, 2026 22:47
@github-actions

Copy link
Copy Markdown

🍁 Maple PR preview

Warning

Preview cleanup could not be confirmed. The Alchemy teardown outcome was skipped.

Final commit f1af5e0 · View workflow run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant