fix: Nudge agents towards macros in raw SQL tiles#2473
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: b5d1e4e The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
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 |
🟡 Tier 3 — StandardIntroduces new logic, modifies core functionality, or touches areas with non-trivial risk. Why this tier:
Review process: Full human review — logic, architecture, edge cases. Stats
|
Greptile SummaryThis PR improves MCP agent guidance for raw SQL dashboard tiles by adding non-blocking advisory warnings when agents create tiles that omit recommended macros (
Confidence Score: 5/5Safe to merge — all warnings are non-blocking advisories; no existing query behaviour is altered. The core logic changes (macro detection, warning generation, wiring into three tools) are well-tested with both unit and integration tests. The only issues are in the new sqlTemplate schema description copy: an inaccurate expansion note for $__fromTime/$__toTime and a SELECT/GROUP BY mismatch in the example query. Neither affects runtime behaviour. packages/api/src/mcp/tools/dashboards/schemas.ts — the $__fromTime/$__toTime expansion description and the example SQL GROUP BY. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Agent calls save/patch/query tile] --> B{Is tile a raw SQL tile?}
B -- No --> C[Skip macro check]
B -- Yes --> D[getRawSqlTileMacroWarnings]
D --> E{Missing time-range macro?}
E -- Yes --> F[Add time-range warning]
E -- No --> G{Time-series display type?}
G -- Yes --> H{Missing interval macro?}
H -- Yes --> I[Add interval warning]
H -- No --> J{Missing filters/sourceTable macros?}
G -- No --> J
J -- Yes --> K[Add filters/sourceTable warning]
J -- No --> L[No warnings]
F --> M[Collect all warnings]
I --> M
K --> M
M --> N{warnings.length > 0?}
N -- Yes --> O[Attach warnings array to response]
N -- No --> P[Return response as-is]
O --> Q[Non-blocking: operation still succeeds]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[Agent calls save/patch/query tile] --> B{Is tile a raw SQL tile?}
B -- No --> C[Skip macro check]
B -- Yes --> D[getRawSqlTileMacroWarnings]
D --> E{Missing time-range macro?}
E -- Yes --> F[Add time-range warning]
E -- No --> G{Time-series display type?}
G -- Yes --> H{Missing interval macro?}
H -- Yes --> I[Add interval warning]
H -- No --> J{Missing filters/sourceTable macros?}
G -- No --> J
J -- Yes --> K[Add filters/sourceTable warning]
J -- No --> L[No warnings]
F --> M[Collect all warnings]
I --> M
K --> M
M --> N{warnings.length > 0?}
N -- Yes --> O[Attach warnings array to response]
N -- No --> P[Return response as-is]
O --> Q[Non-blocking: operation still succeeds]
Reviews (3): Last reviewed commit: "Merge branch 'main' into drew/mcp-warnin..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
E2E Test Results✅ All tests passed • 198 passed • 3 skipped • 1386s
Tests ran across 4 shards in parallel. |
pulpdrew
force-pushed
the
drew/mcp-warning-missing-macros
branch
from
c52f0ac to
820ffab
Compare
| /** | ||
| * True if the SQL contains at least one occurrence of the `$__<name>` macro. | ||
| */ | ||
| export function hasMacro(sql: string, name: string): boolean { | ||
| return findMacros(sql, name).length > 0; | ||
| } | ||
|
|
There was a problem hiding this comment.
backported from EE repo
pulpdrew
requested review from
a team and
karl-power
and removed request for
a team
| - $__timeFilter_ms(col) is the same but should be used when col has millisecond precision (DateTime64 type) | ||
| - $__dateFilter(col) is the same but should be used when col has Day granularity (Date type) | ||
| - $__dateTimeFilter(dateCol, dateTimeCol) should be used when there are both Date and DateTime columns that should be filtered on. | ||
| - NEVER hardcode a fixed time range unless the user specifically asks for it. |
There was a problem hiding this comment.
I'm wondering if this is too strict? I would think a fixed time range is useful when investigating an event
There was a problem hiding this comment.
IMO this is correct for these schemas, since they're only for dashboard building tools, not general incident investigation or notebooks. Even when building dashboards for an investigation, I would still think it makes sense to include these macros so that the time input to works on the dashboard. A user can always set the time range in links they share with others. I can soften the language though if you think there are legitimate cases for fixed-time-range dashboard tiles (even though those aren't really possible with builder tiles)
There was a problem hiding this comment.
Nope makes sense! Thanks for the clarification
2 participants
Summary
This PR improves the MCP schema descriptions for SQL chart tiles and adds warnings when agents create SQL chart tiles without recommended / required macros.
Screenshots or video
How to test
Test locally by connecting to the MCP server and asking claude to build some dashboards.
References