feat: Warn on missing params/macros in SQL Editor

[pulpdrew]

Summary

This PR adds additional validation to the Raw SQL Chart Editor. Previously, missing macro / param warnings only included the validations that were applied when an alert was added to the tile. Now we surface various missing macro / para warnings and errors regardless of whether an alert has been added.

Screenshots or video

How to test on Vercel preview

• Create a raw SQL chart
• Test various queries missing macros or params required/suggested by the display type.

References

• Linear Issue: Closes HDX-4887
• Related PRs:

[changeset-bot]

🦋 Changeset detected

Latest commit: abc11c0

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages

Name	Type
@hyperdx/app	Patch
@hyperdx/common-utils	Patch
@hyperdx/api	Patch
@hyperdx/otel-collector	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
hyperdx-oss	Ready	Preview	Jul 29, 2026 8:19pm
hyperdx-storybook	Ready	Preview	Jul 29, 2026 8:19pm

[github-actions]

🟡 Tier 3 — Standard

Introduces new logic, modifies core functionality, or touches areas with non-trivial risk.

Why this tier:

• Diff size: 254 production lines changed (Tier 2 max: < 250)
• Cross-layer change: touches frontend (packages/app) + backend (packages/api) + shared utils (packages/common-utils)

Review process: Full human review — logic, architecture, edge cases.
SLA: First-pass feedback within 1 business day.

Stats

• Production files changed: 5
• Production lines changed: 254 (+ 373 in test files, excluded from tier calculation)
• Branch: drew/sql-warnings
• Author: pulpdrew

To override this classification, remove the review/tier-3 label and apply a different review/tier-* label. Manual overrides are preserved on subsequent pushes.

[greptile-apps]

Greptile Summary

This PR adds editor-level validation for raw SQL chart parameters and macros.

• Surfaces chart errors and warnings in the raw SQL editor after a short debounce.
• Adds shared validation for time-range, interval, source-dependent, and metrics-source macro usage.
• Reuses source-dependent macro detection in MCP dashboard validation and adds unit coverage.

Confidence Score: 4/5

The PR does not yet appear safe to merge because non-parameter occurrences of parameter names can still bypass the validation this change introduces.

The time-range status helper checks parameter-name substrings in resolved SQL, so comments, literals, or longer identifiers can make missing time-range and interval parameters appear present and suppress the corresponding editor diagnostics.

Files Needing Attention: packages/common-utils/src/core/utils.ts

Important Files Changed

Filename	Overview
packages/common-utils/src/core/utils.ts	Adds shared raw SQL chart validation and refactors alert validation around common time-range status detection.
packages/common-utils/src/macros.ts	Adds helpers for detecting source-dependent macros and counting source-table macro arguments.
packages/app/src/components/ChartEditor/RawSqlChartEditor.tsx	Debounces raw SQL configuration validation and renders resulting warnings and errors in the editor.
packages/api/src/mcp/tools/dashboards/validation.ts	Reuses the shared source-dependent macro helper when validating dashboard tiles.
packages/common-utils/src/tests/validateRawSqlChartConfig.test.ts	Adds broad unit coverage for chart validation across display types, source selection, malformed SQL, and metrics sources.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  SQL[Raw SQL chart config] --> D[Debounced editor config]
  D --> V[validateRawSqlChartConfig]
  V --> T[Time-range and interval checks]
  V --> S[Source macro checks]
  V --> M[Metrics source compatibility checks]
  T --> UI[Editor warning or error alert]
  S --> UI
  M --> UI

Loading

_{Reviews (6): Last reviewed commit: "Merge branch 'main' into drew/sql-warnin..." | Re-trigger Greptile}

[github-actions]

E2E Test Results

✅ All tests passed • 243 passed • 1 skipped • 1051s

Status	Count
✅ Passed	243
❌ Failed	0
⚠️ Flaky	1
⏭️ Skipped	1

Tests ran across 4 shards in parallel.

View full report →

[github-actions]

<!-- deep-review -->

Deep Review

🟡 P2 — recommended

• packages/app/src/components/ChartEditor/RawSqlChartEditor.tsx:144 — the validated config omits metricTables, so replaceMacros throws for any $__sourceTable(<type>) usage, getRawSqlTimeRangeStatus returns null, and both the interval error and the time-filter warning are silently skipped for every metric-source chart.

  • Fix: Populate metricTables on the memoized config from sourceObject when it is a metric source, mirroring how the query path builds it in ChartEditor/utils.ts:245-246.
  • _{correctness-reviewer, adversarial-reviewer, kieran-typescript-reviewer, testing-reviewer}

• packages/common-utils/src/core/utils.ts:1373 — the bare catch { return null; } collapses every replaceMacros failure into "skip all time-range validation", so the new checks go quiet in precisely the malformed states they exist to catch, and validation visibly flickers on once an unrelated condition is fixed.

  • Fix: Derive hasInterval/hasTimeFilter from the raw sqlTemplate using hasMacro over the exported TIME_RANGE_MACROS/INTERVAL_MACROS sets so the checks no longer depend on macro expansion succeeding.
  • _{correctness-reviewer, api-contract-reviewer, testing-reviewer, kieran-typescript-reviewer}

  Worth confirming separately whether this swallow predates the refactor: validateRawSqlForAlert now shares this helper, and packages/api/src/controllers/alerts.ts:78 uses its errors as the server-side gate that rejects an incomplete raw-SQL alert. If the pre-refactor implementation did not swallow, that gate has been weakened; the diff alone cannot settle it.

• packages/common-utils/src/core/utils.ts:1363 — hasInterval/hasTimeFilter substring-match the bare param names rather than the bound {name:Type} form, so a commented-out WHERE clause still satisfies the time-range check and SELECT 60 AS intervalSeconds suppresses the red interval error.

  • Fix: Match the bound form produced by renderQueryParam and strip --//* */ comments and string literals from the resolved SQL before testing.
  • _{adversarial-reviewer, correctness-reviewer}

• packages/common-utils/src/core/utils.ts:1427 — an empty sqlTemplate passes the configType-only guard and replaceMacros('') succeeds, so a fresh time-series raw SQL tile renders a red Error: plus warnings over an untouched editor.

  • Fix: Return empty errors/warnings early when chartConfig.sqlTemplate is blank after trimming.
  • _{correctness-reviewer, adversarial-reviewer}

• packages/common-utils/src/core/utils.ts:1446 — the isDashboardTile warnings never consult chartConfig.from, so a sourceless tile is told to add $__sourceTable/$__filters, and complying flips the banner to the red error at line 1461 that says those macros cannot resolve.

  • Fix: Gate the two source-macro warnings on chartConfig.from being set, or merge both branches into one message that names selecting a source and adding the macro as a single action.
  • _{adversarial-reviewer, correctness-reviewer}

• packages/app/src/components/ChartEditor/utils.ts:388 — validateChartForm consults only validateRawSqlForAlert and only when form.alert is set, so items rendered as red Error: never block save, and the identical condition becomes blocking the moment an alert is attached.

  • Fix: Either feed validateRawSqlChartConfig(...).errors into validateChartForm under the sqlTemplate path, or relabel the chart-level items so the copy does not imply a save gate that is not enforced.
  • _{adversarial-reviewer, correctness-reviewer}

• packages/common-utils/src/core/utils.ts:1433 — this is now the third independent implementation of the same macro rules, and it disagrees with the agent-facing one: packages/api/src/mcp/tools/dashboards/validation.ts:101-108 treats a missing interval macro on a time-series tile as a warning while this treats it as an error, and it detects presence via hasMacro on the template where this substring-matches expanded SQL.

  • Fix: Extract the macro-presence and per-display-type rules into one shared function in common-utils and have both the editor and the MCP validation path consume it.
  • _{maintainability-reviewer, agent-native-reviewer}

• packages/app/src/components/ChartEditor/RawSqlChartEditor.tsx:345 — the new callout passes raw Mantine palette colors ('red'/'yellow') where agent_docs/code_style.md:104 directs new callouts to the themed semantic variants, which it documents for Alert as info | success | warning | danger.

  • Fix: Compute a variant of danger/warning instead of a color and pass it to the Alert.
  • _{project-standards-reviewer}

  The doc calls the variants "opt-in" and leaves existing color= call sites valid, so this is a convention deviation on new code rather than a hard rule break — the stated cost is loss of theme-awareness and inconsistent contrast across brands and light/dark.

• packages/common-utils/src/__tests__/validateRawSqlChartConfig.test.ts:118 — no test drives the getRawSqlTimeRangeStatus → null branch or asserts the validator does not throw on a partially-typed macro, which is why both the silent-skip behaviour and the editor crash shipped uncaught; the one test that incidentally hits the null path asserts only via toContain on an unrelated message.

  • Fix: Add cases for an unterminated $__sourceTable( under isDashboardTile: true, for a metrics config using $__sourceTable(gauge), and for an empty template, asserting no throw and the exact resulting arrays.
  • _{testing-reviewer, correctness-reviewer, adversarial-reviewer, kieran-typescript-reviewer}

• packages/app/src/components/ChartEditor/RawSqlChartEditor.tsx:344 — the headline behaviour of this PR, always-on validation rendered without an alert attached, has no component test, so neither the Alert appearing on warnings alone nor the error-over-warning colour precedence is verified.

  • Fix: Add a React Testing Library test asserting the Alert renders for warnings with no alert configured and resolves to the danger treatment when errors are also present.
  • _{testing-reviewer, correctness-reviewer, kieran-typescript-reviewer}

🔵 P3 nitpicks (7)

• packages/app/src/components/ChartEditor/RawSqlChartEditor.tsx:344 — with an alert attached, the same missing-interval defect is reported twice in different wording, once in the new Alert and once as the TileAlertEditor badge tooltip.

  • Fix: Suppress the interval and time-filter entries from the chart-level Alert when alert is set.

• packages/common-utils/src/__tests__/validateRawSqlChartConfig.test.ts:72 — two test names describe a requireSourceMacros option that does not exist; the real parameter is isDashboardTile.

  • Fix: Rename both test descriptions to reference isDashboardTile.

• packages/common-utils/src/core/utils.ts:1355 — isRawSqlSavedChartConfig is checked three times for one call, at lines 1355, 1386, and 1427.

  • Fix: Keep the guard in getRawSqlTimeRangeStatus only and drop it from the two public callers.

• packages/common-utils/src/core/utils.ts:1420 — the guard's early-return branch is unreachable for any type-checking caller, since the parameter is narrowed to RawSqlChartConfig while isRawSqlSavedChartConfig takes the wider SavedChartConfig; the test can only reach it via as unknown as.

  • Fix: Widen the parameter to the union the guard is meant to discriminate, or drop the runtime guard and the cast-only test with it.

• packages/common-utils/src/core/utils.ts:1379 — the { errors: string[]; warnings: string[] } shape is repeated as an inline literal at lines 1379 and 1420 and destructured twice more in the editor.

  • Fix: Export a single named result type and reference it at all four sites.

• packages/app/src/components/ChartEditor/RawSqlChartEditor.tsx:347 — List.Item is keyed on the message text, which collides silently if two checks ever emit identical copy into the same array.

  • Fix: Key on the array index or on a stable per-message code.

• packages/common-utils/src/__tests__/validateRawSqlChartConfig.test.ts:94 — the isDashboardTile cases assert only with toContain/not.toContain, so a spurious extra warning would pass unnoticed.

  • Fix: Assert the full warnings array with toEqual, matching the exact-array style already used for the interval cases.

---

Reviewers (9): correctness, adversarial, testing, maintainability, project-standards, kieran-typescript, api-contract, agent-native, learnings-researcher.

Testing gaps:

• The catch → null branch of getRawSqlTimeRangeStatus is untested, so both the silent-skip behaviour and the editor crash were unguarded.
• No fixture anywhere sets metricTables, leaving the entire $__sourceTable(metricType) path unexercised through either validator.
• validateRawSqlForAlert has no direct unit test in packages/common-utils/src/__tests__/utils.test.ts; its only coverage is indirect via validateChartForm, and its warning message is never asserted at all — thin ground for a refactor that also backs the server-side alert gate.
• No test combines isDashboardTile: true with from: undefined, the combination that produces the contradictory warning-then-error sequence.

Scope caveat: git, grep, and glob were all unavailable in this environment, so no true git diff against the base SHA was obtainable. Scope was reconstructed by enumerating .git/index and reading files directly, then confirmed against .changeset/warn-raw-sql-macros.md. Consequently, pre_existing attribution is inferred from code shape rather than history — this affects the catch → null finding in particular, and the substring-matching behaviour appears carried over from validateRawSqlForAlert rather than introduced here.

[github-actions]

<!-- deep-review -->

Deep Review

⚠️ Scope note: git/bash and all search tools were unavailable in this run (sandbox init failure: bwrap: Can't create file at /home/.mcp.json), so no unified diff could be obtained. Scope was reconstructed from the changeset (.changeset/warn-raw-sql-macros.md) and direct reads of the feature surface. Findings below were verified against the actual file contents; pre-existing behavior was excluded where identifiable, but line-level attribution of new-vs-existing code is less certain than in a diff-backed review.

✅ No critical issues found.

🟡 P2 -- recommended

• packages/app/src/components/ChartEditor/RawSqlChartEditor.tsx:138 -- The rawSqlConfig memo omits metricTables, so for a metrics source replaceMacros throws on $__sourceTable(<metricType>), getRawSqlTimeRangeStatus swallows the throw and returns null, and every interval/time-filter diagnostic this PR adds is silently skipped.
  • Fix: Populate metricTables from the selected source in the memo (isMetricSource is already imported at line 18) and add sourceObject to the dependency list.
  • _{correctness, kieran-typescript}
• packages/common-utils/src/core/utils.ts:1420 -- validateRawSqlChartConfig and its helper getRawSqlTimeRangeStatus are the headline logic of this change and have no unit coverage: packages/common-utils/src/__tests__/utils.test.ts imports 19 other symbols from core/utils but none of the validation functions, and no RawSqlChartEditor component test exists.
  • Fix: Add unit tests covering the time-series interval error, the time-filter warning, both isDashboardTile macro warnings, the missing-from source-dependent-macro error, and the malformed-macro catch path.
  • _{testing, kieran-typescript}
• packages/common-utils/src/core/utils.ts:1367 -- hasTimeFilter requires both startDateMilliseconds and endDateMilliseconds to be present, so a query bound to the range with a single $__fromTime, $__toTime, $__fromTime_ms, or $__toTime_ms gets the "should include start and end date parameters" warning anyway, contradicting the TIME_RANGE_MACROS doc comment that says any one of them satisfies the check.
  • Fix: Treat either param as satisfying the time-range check, or reword the warning to state that only one side of the range is bound.

🔵 P3 nitpicks (5)

• packages/common-utils/src/core/utils.ts:1363 -- Param detection uses sql.includes(<bare param name>) on the resolved SQL rather than matching the {name:Type} token, so a commented-out param or a column alias such as count() AS startDateMilliseconds satisfies the check with nothing actually bound.
  • Fix: Match the real token with a regex like /\{\s*<name>\s*:/ instead of a bare substring test.
• packages/app/src/components/ChartEditor/RawSqlChartEditor.tsx:150 -- When an alert is attached, validateRawSqlForAlert and validateRawSqlChartConfig both emit an interval message and both emit a time-filter message from the same config, rendering four diagnostics for two root causes, and the alert-path strings omit the (e.g. $__interval_s) / (e.g. $__timeFilter) hints the chart-path strings carry.
  • Fix: Emit the shared interval/time-range checks from one validator and leave validateRawSqlForAlert responsible only for alert-specific checks such as the display-type guard.
  • _{correctness, maintainability}
• packages/common-utils/src/macros.ts:183 -- TIME_RANGE_MACROS and INTERVAL_MACROS are exported with doc comments describing precisely the check getRawSqlTimeRangeStatus performs, yet that function never references them and no file in the reviewed surface imports them; knip.json treats every src/**/*.ts in this package as an entry point, so the drift will not surface in CI.
  • Fix: Either drive the interval/time-range detection from these constants via hasMacro, or delete them until a consumer exists.
• packages/common-utils/src/core/utils.ts:1441 -- The warning suggests $__timeFilter, but that macro requires a column argument, so a user pasting it verbatim triggers an arity throw that getRawSqlTimeRangeStatus swallows, leaving the same warning on screen with no explanation.
  • Fix: Write the hint as $__timeFilter(<timestamp column>).
• packages/app/src/components/ChartEditor/RawSqlChartEditor.tsx:1 -- The file is 369 lines against the AGENTS.md guidance to keep components under 300, and the new validation is pure derived state over rawSqlConfig and isDashboardForm with no component-local dependencies.
  • Fix: Extract the two validation memos into a useRawSqlChartValidation hook.

---

Reviewers (4): correctness, testing, maintainability, kieran-typescript.

Testing gaps:

• No test exercises validateRawSqlChartConfig with from set but metricTables omitted plus a $__sourceTable(<metricType>) macro — the exact input that silently disables all diagnostics.
• No test feeds malformed macro syntax (unterminated paren) through either validator to confirm the catch paths degrade instead of throwing.
• MACRO_SUGGESTIONS drives editor autocomplete but has no coverage, so a wrong minArgs would silently produce a malformed apply string.
• No test asserts single-sided $__fromTime / $__toTime behavior against the time-filter check.

[github-actions]

Deep Review

✅ No critical issues found. Nothing here is a ship-blocker: the new validation is additive, non-blocking, and defensively wrapped. The substantive concerns are false-negatives that let the feature silently fail to warn, plus a theming-convention violation on the new callout.

🟡 P2 -- recommended

• packages/app/src/components/ChartEditor/RawSqlChartEditor.tsx:353 -- The new callout renders <Alert color={sqlValidationAlertColor}> with raw Mantine palette values 'red'/'yellow', which agent_docs/code_style.md:104-118 lists as a forbidden pattern for new callouts because it is not brand/light-dark aware and does not meet WCAG AA contrast.
  • Fix: Replace the color prop with the themed semantic variants, passing variant="danger" when errors.length > 0 and variant="warning" otherwise.
• packages/common-utils/src/core/utils.ts:1364 -- hasTimeFilter/hasInterval are decided by plain sql.includes(...) on the macro-expanded text, so any occurrence of the bare param name satisfies the check: SELECT count() FROM $__sourceTable -- TODO: use startDateMilliseconds and endDateMilliseconds suppresses the time-range warning for a query that ignores the time range entirely.
  • Fix: Match the full rendered param token (renderQueryParam(...), i.e. {startDateMilliseconds:Int64}) rather than the bare name, after stripping SQL comments and string literals.
• packages/common-utils/src/core/utils.ts:1374 -- getRawSqlTimeRangeStatus returns null for any replaceMacros failure, so both the interval error and the time-range warning vanish for configs that a user can legitimately reach: a metrics source whose metricTables has no entry for the referenced type makes $__sourceTable(gauge) throw (macros.ts:346), and a Line chart with no interval macro then gets zero feedback.
  • Fix: Detect param/macro presence from the unexpanded sqlTemplate (via the existing hasMacro plus a param-token scan) so time-range validation no longer depends on full macro resolution succeeding.
• packages/common-utils/src/core/utils.ts:1488 -- The single try/catch {} spans four independent checks and returns whatever was accumulated before the throw, so results are order-dependent and misleading: sqlTemplate: 'SELECT * WHERE $__filters(' with isDashboardTile: true yields exactly one warning telling the user to add $__sourceTable, while the actual unmatched paren and the missing $__filters guidance are both dropped.
  • Fix: Scope the catch to each individual macro-parsing call so one unparseable macro cannot suppress the remaining checks, and surface a single "SQL could not be parsed" warning instead of a partial list.
  • _{correctness-orchestrator, maintainability}
• packages/app/src/components/ChartEditor/RawSqlChartEditor.tsx:352 -- The new <Alert> block, the isDashboardForm → isDashboardTile wiring, and the debounced recompute have no component-level test; all new coverage lives in pure-function tests against validateRawSqlChartConfig, so a regression that stops rendering the messages entirely would pass CI.
  • Fix: Add a render test alongside packages/app/src/components/DBEditTimeChartForm/__tests__/ asserting the error and warning list items appear for an invalid sqlTemplate and clear once the required macros are added.

🔵 P3 nitpicks (8)

• packages/common-utils/src/macros.ts:183 -- TIME_RANGE_MACROS and INTERVAL_MACROS are exported with multi-sentence docstrings prescribing how callers "should" use them, but nothing imports them; knip.json:28 treats every packages/common-utils/src/**/*.ts file as an entry point, so the dead exports will not be flagged by tooling.
  • Fix: Delete both constants, or refactor getRawSqlTimeRangeStatus to derive presence by iterating them so the documented intent has exactly one implementation.
  • _{maintainability, correctness-orchestrator}
• packages/common-utils/src/__tests__/validateRawSqlChartConfig.test.ts:72 -- Two test titles describe a requireSourceMacros option that does not exist anywhere in the codebase; the assertions actually pass { isDashboardTile: false } and { isDashboardTile: true }.
  • Fix: Rename both test titles to reference isDashboardTile.
  • _{maintainability, correctness-orchestrator}
• packages/app/src/components/ChartEditor/RawSqlChartEditor.tsx:165 -- A Line tile with an alert and no interval macro shows two near-identical messages simultaneously: the new "SQL must include an interval parameter or macro (e.g. $__interval_s) for this display type." in the callout, and "SQL used for alerts must include an interval parameter or macro." inside TileAlertEditor.
  • Fix: Suppress the alert-specific interval/time-range messages when the chart-level callout already reports the same condition.
• packages/app/src/components/ChartEditor/utils.ts:388 -- validateChartForm never calls validateRawSqlChartConfig, so items the callout labels Error: (including "no source is selected", which guarantees a render-time throw) do not block submission, making the Error/Warning distinction meaningless to the user.
  • Fix: Either relabel the chart-level items as warnings, or feed validateRawSqlChartConfig().errors into validateChartForm so Error: genuinely gates save.
• packages/app/src/components/ChartEditor/RawSqlChartEditor.tsx:157 -- validateRawSqlForAlert and validateRawSqlChartConfig each call getRawSqlTimeRangeStatus independently, so every debounced keystroke performs two full replaceMacros expansions (each sorting the macro list and running a fresh regex per macro) plus three more whole-string scans, and the alert expansion runs even when no alert is configured.
  • Fix: Compute the shared time-range status once per debounced config and pass it into both validators, and skip the alert validation entirely when alert is undefined.
• packages/common-utils/src/core/utils.ts:1436 -- Full user-facing English sentences are embedded in common-utils, a shared server-importable package, coupling UI copy to a library consumed well beyond the SQL editor.
  • Fix: Return stable message codes plus structured detail from the validator and map them to display strings in the app layer.
  • _{maintainability, correctness-orchestrator}
• packages/common-utils/src/core/utils.ts:1424 -- The { errors: string[]; warnings: string[] } shape is now repeated inline in three signatures, and the new test file reaches for as RawSqlChartConfig / as unknown as RawSqlChartConfig to build inputs the declared parameter type forbids, against the agent_docs/code_style.md:8 guidance to avoid as casts.
  • Fix: Extract a named ChartConfigValidationResult type and widen the validator parameter to the union the guard actually narrows so the test casts become unnecessary.
• packages/app/src/components/ChartEditor/RawSqlChartEditor.tsx:112 -- The component is now 377 lines, past the 300-line ceiling stated in AGENTS.md:88 and agent_docs/code_style.md:17; the file was already over the limit and this change pushes it further.
  • Fix: Extract the validation memos and the callout into a small RawSqlValidationAlert component or a useRawSqlValidation hook.

---

Reviewers (3): correctness-orchestrator (direct verification of every cited line), maintainability, learnings-researcher.

Coverage caveats:

• Bash, git, gh, Grep, and Glob were all unavailable in this environment (sandbox init failure), so no git diff against 8aeb2f32 was possible; findings were derived by reading the working-tree files and attributing new code from the changeset, function structure, and the new test file. Pre-existing-vs-new attribution on validateRawSqlForAlert and replaceMacros internals is therefore best-effort — treat the two core/utils.ts false-negative findings as applying to the new validateRawSqlChartConfig path regardless of whether the alert path shared the flaw before.
• Eight further reviewers (adversarial, testing, project-standards, kieran-typescript, frontend-races, performance, agent-native, plus the learnings pass) were dispatched; only the two named above returned before this report was assembled, so adversarial SQL-input fuzzing and the agent-native/MCP parity question are unverified.

Testing gaps:

• No test covers validateRawSqlForAlert after its refactor onto the shared getRawSqlTimeRangeStatus helper, so the alert-path regression risk from that extraction is uncovered.
• hasMacro has word-boundary coverage in macros.test.ts, but the new getSourceTableMacroArgCounts has no direct tests, and no test exercises macros appearing inside SQL string literals or -- / /* */ comments.
• No coverage for displayType === undefined or for display types whose param list is empty (Search, Heatmap, Markdown, EventPatterns in rawSqlParams.ts:95-98), where the time-range warning still fires.
• No server-side write path was confirmed to call validateRawSqlChartConfig, so an agent or External API client creating the same raw SQL tile may persist a configuration the UI would flag.

[pulpdrew]

These P0s are existing issues and beyond the scope of this PR.

[teeohhem]

Looks great!

[github-actions]

Deep Review

✅ No critical issues found. The new validateRawSqlChartConfig path is well covered by packages/common-utils/src/__tests__/validateRawSqlChartConfig.test.ts (23 cases, including macro-parse failure degradation), and the Alert variant="danger|warning" switch is the documented semantic-variant API per agent_docs/themes.md. The issues below are UX/consistency and coverage gaps.

🟡 P2 -- recommended

• packages/common-utils/src/core/utils.ts:1421 -- validateRawSqlChartConfig has no empty-template guard, so a freshly created raw SQL chart reports errors and warnings before the user has typed anything.
  • Fix: Return early with empty errors/warnings when chartConfig.sqlTemplate.trim() is empty, so validation only fires once there is SQL to validate.
• packages/app/src/components/ChartEditor/RawSqlChartEditor.tsx:352 -- chartErrors are rendered with an Error: prefix in a danger alert, but validateChartForm only wires validateRawSqlForAlert into setError and never calls validateRawSqlChartConfig, so a tile displaying Error: SQL uses $__sourceTable but no source is selected still saves and then throws at query time.
  • Fix: Route validateRawSqlChartConfig errors through validateChartForm via setError('sqlTemplate', …) so they block submission, or downgrade them to warnings to match their non-blocking behavior.
• packages/common-utils/src/core/utils.ts:1448 -- The isDashboardTile warnings tell the user to add $__sourceTable and $__filters, but lines 1463-1469 turn those same macros into an error when no source is selected, and the Source field is explicitly optional in the editor — a tile that hardcodes its table gets two permanently unsatisfiable warnings.
  • Fix: Gate the isDashboardTile source-macro warnings on chartConfig.from being set, or extend the message to say a source must be selected first.
• packages/app/src/components/ChartEditor/RawSqlChartEditor.tsx:349 -- The new validation surface has no component test; ChartEditor/__tests__/ contains only resolveConnectionSourceSync.test.ts and utils.test.ts, so the errors.length > 0 ? 'danger' : 'warning' variant choice, the Error:/Warning: prefixes, and the debounced wiring can all regress silently.
  • Fix: Add a RawSqlChartEditor render test asserting the alert variant and the message list for an errors-present case and a warnings-only case.

🔵 P3 nitpicks (3)

• packages/common-utils/src/core/utils.ts:1435 -- validateRawSqlChartConfig and validateRawSqlForAlert derive interval/date-range messages from the same getRawSqlTimeRangeStatus result with different wording, so a Line chart with an alert and no interval macro shows the same defect twice (inline list plus the Invalid Query badge).
  • Fix: Have the alert path skip the interval/date-range messages already emitted by the chart-level validator and keep only alert-specific text.
• packages/common-utils/src/__tests__/validateRawSqlChartConfig.test.ts:72 -- Two test descriptions refer to a requireSourceMacros option, but the implemented option is isDashboardTile.
  • Fix: Rename both descriptions to reference isDashboardTile.
• packages/common-utils/src/core/utils.ts:1365 -- hasInterval/hasTimeFilter use bare sql.includes() on the macro-resolved SQL, so a column alias or comment containing startDateMilliseconds, endDateMilliseconds, or intervalSeconds silently suppresses the warning.
  • Fix: Match the rendered param form (e.g. {startDateMilliseconds:Int64}) via renderQueryParam rather than the bare param name.

---

Reviewers (0): the plugin's persona sub-agent fan-out did not run. Bash failed on every invocation in this environment (bwrap: Can't create file at /home/.mcp.json, including echo and with the sandbox override), WebFetch was denied, and no Grep/Glob tool was available to this session or to sub-agents — the one dispatched explorer returned an environment blocker rather than results. Scope was reconstructed from .git/index plus .changeset/warn-raw-sql-macros.md (which scopes the change to @hyperdx/app and @hyperdx/common-utils), then reviewed inline via direct file reads of core/utils.ts, macros.ts, rawSqlParams.ts, guards.ts, RawSqlChartEditor.tsx, RawSqlChartInstructions.tsx, ChartEditor/utils.ts, TileAlertEditor.tsx, and the new test file. Findings not derived from a unified diff cannot be attributed line-by-line to added versus pre-existing lines, so treat the pre-existing/new split as inferred; TileAlertEditor.tsx in particular may be untouched by this PR and was used only as corroboration.

Testing gaps:

• No component-level test for the new chart-editor warning/error surface in RawSqlChartEditor.
• No validateRawSqlChartConfig case for an empty or whitespace-only sqlTemplate.
• No case covering a partial time-range macro (e.g. $__fromTime without $__toTime), where only one of the two required params resolves.

[github-actions]

Deep Review

_{Note: git, grep, and glob were unavailable in this environment (sandbox bwrap failure), so the diff could not be read directly. Scope was reconstructed from the branch's two commits by reading the working tree; findings cite verified code at HEAD, and pre-existing labels are inferred rather than confirmed against the base.}

🔴 P0/P1 -- must fix

• packages/app/src/components/ChartEditor/RawSqlChartEditor.tsx:145 -- from: sourceObject?.from is undefined on first paint because useSources() is a bare useQuery with no placeholderData, so any saved chart whose SQL uses $__sourceTable/$__filters renders a false red "no source is selected" error until the sources query resolves.
  • Fix: Pass a sourcesLoaded signal (or skip validation entirely while sources === undefined) so the !chartConfig.from branch at core/utils.ts:1463 cannot fire before source data has arrived.
  • _{frontend-races}

🟡 P2 -- recommended

• packages/common-utils/src/core/utils.ts:1374 -- getRawSqlTimeRangeStatus returns null whenever replaceMacros throws, and because if (status) guards both checks, a metrics source with $__sourceTable(gauge) whose metricTables lacks gauge produces no error and no warning at all — the arg-count checks at 1476/1482 also pass it.
  • Fix: Return a distinct "unresolved" state carrying the caught message and surface it as an error once the debounced template has settled, rather than collapsing it into the same null as "not raw SQL".
  • _{correctness, adversarial, api-contract}
• packages/common-utils/src/core/utils.ts:1488 -- one try spans all ~55 lines of independent checks, so a single unmatched paren in $__sourceTable( makes hasMacro throw at line 1449 and silently discards the $__filters warning, the no-source error, and both metrics mismatch errors.
  • Fix: Wrap each independent check in its own guard, or make hasMacro/getSourceDependentMacrosUsed/getSourceTableMacroArgCounts return a safe default instead of throwing on unparseable args.
  • _{correctness, adversarial, frontend-races}
• packages/common-utils/src/core/utils.ts:1428 -- there is no empty-template guard, so a fresh Line tile with sqlTemplate: '' immediately renders one error plus three warnings about SQL the user has not written yet, since useDebouncedValue seeds synchronously and replaceMacros('') does not throw.
  • Fix: Return empty results early when chartConfig.sqlTemplate?.trim() is falsy, leaving the existing "SQL query is required" form validation to cover emptiness.
  • _{correctness, adversarial, frontend-races}
• packages/common-utils/src/core/utils.ts:1368 -- hasInterval/hasTimeFilter use sql.includes() against the bare param identifiers, so a commented-out -- ... {startDateMilliseconds:Int64} ... {endDateMilliseconds:Int64} line satisfies the check and the chart silently ignores the selected time range with no warning.
  • Fix: Match the rendered {name:Type} token shape produced by renderQueryParam, and strip SQL comments and string literals before testing.
  • _{correctness, adversarial}
• packages/common-utils/src/core/utils.ts:1448 -- a dashboard tile with no source has no warning-free state: omitting the macros yields two "should include" warnings, adding them flips to the error at 1466, and $__filters is classified as an error even though macros.ts:167 documents that it degrades to (1=1) rather than failing.
  • Fix: Gate the tile-specific $__sourceTable/$__filters warnings on chartConfig.from being set, and downgrade the no-source $__filters case from error to warning.
  • _{correctness, adversarial}
• packages/common-utils/src/core/utils.ts:1436 -- extending the interval requirement beyond the alert path means a valid Line chart using a literal INTERVAL 1 MINUTE bucket is reported as an error, since only the macro/param form is accepted.
  • Fix: Demote the non-alert interval finding to a warning explaining that the chart will not follow the granularity selector, keeping the hard error scoped to validateRawSqlForAlert.
  • _adversarial
• packages/common-utils/src/core/utils.ts:1421 -- validateRawSqlChartConfig is the headline function of this change and has no test anywhere reachable, leaving all eight of its error/warning outcomes plus both silent-swallow paths unexercised; getSourceTableMacroArgCounts is likewise untested.
  • Fix: Add a describe('validateRawSqlChartConfig') block in packages/common-utils/src/__tests__/utils.test.ts covering each outcome and both catch paths, plus a getSourceTableMacroArgCounts block in macros.test.ts.
  • _{testing, correctness, maintainability, api-contract}

🔵 P3 nitpicks (7)

• packages/common-utils/src/macros.ts:183 -- TIME_RANGE_MACROS and INTERVAL_MACROS are exported with detailed docs but have no consumer, and the docblock's claim that "the presence of any of these satisfies a time-range check" contradicts hasTimeFilter, which requires both the start and end params.
  • Fix: Delete both constants, or wire getRawSqlTimeRangeStatus to them and correct the docblock to describe the actual AND semantics.
• packages/common-utils/src/core/utils.ts:1401 -- extracting getRawSqlTimeRangeStatus deduplicated only the boolean computation; both validators still carry their own copies of the same two checks with textually different user-facing strings that must now be kept in sync by hand.
  • Fix: Extract a shared message-builder both functions call, parameterised on the alert-vs-chart wording.
• packages/app/src/components/DBEditTimeChartForm/TileAlertEditor.tsx:124 -- the error Badge moved to var(--color-text-danger) while its sibling warning Badge stayed on the raw palette color="yellow", despite --color-text-warning being defined in _tokens.scss.
  • Fix: Change the warning Badge to color="var(--color-text-warning)" to finish the token migration.
• packages/app/src/components/ChartEditor/RawSqlChartEditor.tsx:157 -- the two adjacent memos each call a validator that internally runs replaceMacros, so the full macro scan and replace executes twice per settled keystroke.
  • Fix: Compute the shared time-range status once in a single memo and have both validators build their messages from that result.
• packages/app/src/components/ChartEditor/RawSqlChartEditor.tsx:353 -- the new messages are prefixed "Error:" but are never routed through setError, so nothing they describe actually blocks saving the chart.
  • Fix: Either feed chartErrors into form validation so they gate submission, or relabel them as advisory to match their real effect.
• packages/common-utils/src/core/utils.ts:1423 -- the { isDashboardTile } option is supplied from the caller's isDashboardForm prop, so neither name can be traced to the other, and the single-boolean options object earns nothing over a plain parameter.
  • Fix: Align the two names on one term and drop the wrapper object until a second option exists.
• packages/app/src/components/ChartEditor/utils.ts:389 -- pre-existing: validateChartForm builds its RawSqlChartConfig without metricTables, so the same SQL validates differently here than in the editor memo, which does include it.
  • Fix: Extract one shared config builder and use it at both call sites.

---

Reviewers (7): correctness, adversarial, testing, maintainability, project-standards, frontend-races, api-contract

Testing gaps:

• validateRawSqlChartConfig has zero coverage; packages/common-utils/src/__tests__/utils.test.ts does not import it or validateRawSqlForAlert.
• Neither swallowing catch (core/utils.ts:1374, :1488) is pinned by a test, so "broken SQL yields no diagnostics" can flip in either direction unnoticed.
• No test covers the empty-sqlTemplate case or the unmatched-paren path that triggers the outer catch.
• No component test covers the new <Alert> block or the mount-time window where useSources() has not yet resolved.
• macros.test.ts:58 asserts ['filters','sourceTable'], coupling the test to SOURCE_DEPENDENT_MACROS declaration order rather than to behavior.
• Not verifiable here: whether a .changeset/ entry exists (required by AGENTS.md for user-facing @hyperdx/app changes) — directory enumeration was unavailable.