Skip to content

fix(mcp): steer agents toward builder query tools instead of raw SQL (HDX-4892) - #2840

Open
brandon-pereira wants to merge 9 commits into
mainfrom
brandon/brandon-mcp-sql-tool-use-investigation
Open

fix(mcp): steer agents toward builder query tools instead of raw SQL (HDX-4892)#2840
brandon-pereira wants to merge 9 commits into
mainfrom
brandon/brandon-mcp-sql-tool-use-investigation

Conversation

@brandon-pereira

@brandon-pereira brandon-pereira commented Aug 8, 2026

Copy link
Copy Markdown
Member

Summary

We've noticed agents increasingly reaching for raw SQL instead of the structured builder query tools — drift that crept in as the tool set grew without a clear, up-front tool-selection policy. Over-using clickstack_sql produces static result tiles instead of the builder tiles users can click into and pivot from, and it's more error-prone.

This PR steers agents back toward the builder tools (clickstack_table / clickstack_timeseries / clickstack_search) and reserves raw SQL for cases the builders genuinely can't express. It also includes an eval-harness fix (MCP reachability preflight) that was needed to validate the steering change with trustworthy A/B evals — see the scoping note below.

Scoping note for review: this PR contains two logically distinct changes — the MCP steering (packages/api) and the eval preflight (packages/hdx-eval). I kept them together because the preflight fix is what unblocked a valid A/B of the steering change, and those eval results are the primary justification for the steering change. Happy to split into two PRs if you'd prefer.

Why we needed this (HDX-4892)

Production MCP telemetry confirmed the drift. Over the last 30 days of investigation traffic:

  • clickstack_sql was ~73% of all data-querying calls — ~6.5× the next query tool.
  • Raw SQL also had the highest error rate (~16%, roughly 2× the builder tools), so agents weren't fleeing failing builder tools — they were defaulting to the low-friction, broadly-advertised SQL path. Each failed SQL call tends to trigger a retry (another SQL call), compounding the skew.

The root cause was steering, not capability:

  • The MCP server passed no server-level instructions — there was no tool-selection policy the agent saw by default.
  • The only "prefer builder tools" hint lived in the clickstack_sql description and was hedged ("ADVANCED: only use this when…"). None of the builder tools asserted primacy over SQL.
  • The strongest guidance lived in the opt-in query_guide / create_dashboard prompts, which a free-form investigation agent never fetches.

What changed

MCP steering (packages/api/src/mcp/)

  • sql.ts — reframed clickstack_sql as a last-resort tool with an explicit decision rule: single-source aggregations, top-N, time-series, and row browses must use builder tools; SQL is only for JOINs / sub-queries / CTEs / unregistered tables.
  • table.ts, timeseries.ts, search.ts — added a reciprocal "prefer me over clickstack_sql" nudge so the steering isn't one-directional.
  • mcpServer.ts — added a server-level instructions tool-selection policy, surfaced on the MCP initialize handshake so agents see it by default instead of only via an opt-in prompt.

Eval harness preflight (packages/hdx-eval/)

  • New harness/preflight.ts: probes every MCP server (initialize + tools/list) before spawning any agents, and the run command aborts the batch if a server is unreachable or serves zero tools. Added --no-preflight to bypass.
  • Unit tests in __tests__/preflight.test.ts (SSE + plain-JSON bodies, zero-tools, HTTP error, connection-refused, JSON-RPC error, stdio passthrough).

Why the preflight fix was necessary

The first full A/B run came back with all-flat, near-zero scores. On inspection, 0 of 59 runs made a single MCP tool call — the dev API servers had died mid-suite, so Claude Code's MCP client got a dead endpoint, every agent burned its turns on ToolSearch trying to discover tools that were never served, and the runs graded as real (but terrible) scores. A dead server silently masquerading as "the model did badly" is a nasty failure mode; the preflight now fails loudly and immediately with an actionable message ("is the API server on this slot running?").

How it affects the evals

Re-ran the full hdx-eval suite (8 scenarios) branch vs main, dual-slot (main = slot 98, branch = slot 99), identical seeded data on both, judge = claude-opus-4-7. Combined score (Δ = branch − main). The two scenarios with a "major" runs=3 delta were re-run at runs=5 to rule out variance:

Scenario Branch Main Δ N
dashboard-build 87% 51% +37% 5
latency-spike 78% 71% +7% 3
segmented-regression 79% 75% +4% 5
noisy-signals 67% 63% +4% 3
metric-saturation 64% 63% +2% 3
error-root-cause 96% 95% +1% 3
service-health-check 55% 59% −4% 3
deploy-regression 76% 81% −5% 3

Takeaways:

  • Big, robust win on dashboard-build: +37% (judge mean 85% vs 25%), confirmed at runs=5. This is the scenario most sensitive to builder-vs-SQL tile choice — agents now build proper builder tiles instead of dumping raw-SQL tiles.
  • Branch wins or ties 6 of 8 scenarios.
  • segmented-regression looked like a −9% regression at runs=3 but flipped to +4% at runs=5 — it was variance, not a real regression. The two remaining small negatives (−4%, −5%) are within runs=3 noise.
  • Efficiency held or improved: branch used fewer or comparable tool calls with equal-or-fewer tool errors in most scenarios.

Testing

  • packages/api: MCP query unit tests pass; tsc --noEmit clean; yarn lint:fix clean.
  • packages/hdx-eval: ci:lint (eslint + tsc) clean; new preflight unit suite passes (8 tests).
  • End-to-end: preflight verified in both directions (live server → OK (29 tools); dead port → loud abort). Full 8-scenario A/B + runs=5 confirmations executed against live stacks.

Probe each MCP server (initialize + tools/list) before spawning any
agents and abort the batch if a server is unreachable or serves zero
tools. Previously a dead API server produced an entire suite of silent
zero-tool-call runs that graded as real (bad) scores, masking the
failure. Add --no-preflight to bypass.
@changeset-bot

changeset-bot Bot commented Aug 8, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 6b82767

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

This PR includes changesets to release 3 packages
Name Type
@hyperdx/api Patch
@hyperdx/app 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

vercel Bot commented Aug 8, 2026

Copy link
Copy Markdown

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

2 Skipped Deployments
Project Deployment Actions Updated (UTC)
hyperdx-oss Ignored Ignored Preview Aug 10, 2026 4:29pm
hyperdx-storybook Ignored Ignored Preview Aug 10, 2026 4:29pm

Request Review

@greptile-apps

greptile-apps Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR adds agent-facing guidance that prioritizes structured MCP query builders over raw SQL and introduces an eval-harness MCP reachability preflight.

  • Centralizes builder-tool and SQL fallback guidance and exposes it through server initialization and tool descriptions.
  • Probes configured HTTP MCP servers with initialize and tools/list, aborting before scenario seed detection or reseeding when a server is unavailable.
  • Adds preflight coverage for supported response formats and failure conditions.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
packages/api/src/mcp/mcpServer.ts Adds server-level tool-selection instructions to the MCP initialize response.
packages/api/src/mcp/tools/query/builderCatalog.ts Centralizes builder-tool descriptions and the criteria for falling back to raw SQL.
packages/hdx-eval/src/harness/preflight.ts Implements HTTP MCP initialization and tool-list reachability checks while passing through stdio definitions.
packages/hdx-eval/src/cli.ts Runs the preflight before seed detection and reseeding, fully addressing the prior ordering issue.
packages/hdx-eval/src/tests/preflight.test.ts Covers successful JSON/SSE responses, zero-tool and protocol failures, connectivity errors, and stdio passthrough.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  Run[Start eval run] --> Resolve[Resolve config and anchor time]
  Resolve --> Preflight[Probe HTTP MCPs]
  Preflight -->|Failure or zero tools| Abort[Abort batch]
  Preflight -->|All healthy| SeedCheck[Check or reseed scenario data]
  SeedCheck --> Agents[Spawn evaluation agents]
  Agents --> Grade[Grade and report results]
Loading

Reviews (6): Last reviewed commit: "Merge branch 'main' into brandon/brandon..." | Re-trigger Greptile

Comment thread packages/hdx-eval/src/cli.ts Outdated
@github-actions

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

E2E Test Results

All tests passed • 277 passed • 1 skipped • 1117s

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

Tests ran across 4 shards in parallel.

View full report →

@brandon-pereira
brandon-pereira marked this pull request as ready for review August 10, 2026 14:59
@github-actions github-actions Bot added the review/tier-2 Low risk — AI review + quick human skim label Aug 10, 2026
@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

🔵 Tier 2 — Low Risk

Small, isolated change with no API route or data model modifications.

Why this tier:

  • Standard feature/fix — introduces new logic or modifies core functionality

Additional context: 2 file(s) in private internal-tooling packages, excluded from the line count

Review process: AI review + quick human skim (target: 5–15 min). Reviewer validates AI assessment and checks for domain-specific concerns.
SLA: Resolve within 4 business hours.

Stats
  • Production files changed: 8
  • Production lines changed: 99 (+ 73 in test files, excluded from tier calculation)
  • Branch: brandon/brandon-mcp-sql-tool-use-investigation
  • Author: brandon-pereira

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

Move the preflight probe ahead of the re-seed block so an unavailable
MCP server fails fast instead of after truncating and repopulating
scenario tables (--reseed/--live). The preflight is independent of
seeding, so running it first avoids the avoidable destructive work.

Addresses Greptile review feedback on #2840.
@vercel
vercel Bot temporarily deployed to Preview – hyperdx-storybook August 10, 2026 15:03 Inactive
@brandon-pereira
brandon-pereira marked this pull request as draft August 10, 2026 15:03
@brandon-pereira
brandon-pereira marked this pull request as ready for review August 10, 2026 15:04
@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Deep Review

Scope: 8351d632…HEAD — 11 files. Two logically distinct changes: (1) prose steering of the ClickStack MCP tools toward builder query tools over raw SQL (packages/api/src/mcp/**), and (2) a new MCP reachability preflight for the hdx-eval harness (packages/hdx-eval/**).

Intent: Reduce agent over-reliance on clickstack_sql by reframing it as last-resort, adding reciprocal builder-tool nudges and a server-level instructions policy; add a preflight that aborts an eval batch when an HTTP MCP server is unreachable or serves zero tools.

✅ No critical issues found. The happy paths (server instructions surfaced on initialize, successful probe returning a tool count) are sound and tested; the change carries no auth, injection, data-loss, or crash-on-happy-path risk. Recommendations below.

🟡 P2 -- recommended

  • packages/api/src/mcp/tools/query/builderCatalog.ts:8 -- BUILDER_TOOLS_LIST advertises 8 builder tools, but PREFER_BUILDER_OVER_SQL_NUDGE is only imported by 5 (table, timeseries, search, eventDeltas, eventPatterns); emergingSignals.ts, trace/waterfall.ts, and trace/breakdown.ts omit it, contradicting the file's own doc comment ("dropped into each builder tool's description").
    • Fix: Add PREFER_BUILDER_OVER_SQL_NUDGE to clickstack_emerging_signals and both trace tools, or narrow the doc comment and list to the tools that actually carry it.
    • maintainability, orchestrator-verified
  • packages/hdx-eval/src/cli.ts:633 -- The core new behavior (aggregate probe results, log per-MCP status, and throw to abort the batch when any probe fails) has no test, and the error branches in preflight.ts (HTTP non-2xx, JSON-RPC error, timeout/AbortError) are likewise unexercised.
    • Fix: Extract the probe-result-to-abort decision into a pure helper and unit-test both the all-ok pass-through and the failed > 0 throw, plus add probeMcp tests for the HTTP-error, JSON-RPC-error, and timeout branches.
    • testing
  • packages/hdx-eval/src/harness/preflight.ts:172 -- The probe performs a single initialize + tools/list with no retry, so a transient network blip, a cold-starting-but-alive server, or a long GC pause surfaces as a connection/abort error and aborts the entire suite — the same wasted-batch outcome the feature exists to prevent, now triggered by a false negative.
    • Fix: Add 1–2 retries with short backoff before declaring a server unreachable so only a persistently-down server aborts.
    • reliability
🔵 P3 nitpicks (5)
  • packages/hdx-eval/src/harness/preflight.ts:160 -- When parseJsonRpcBody throws or the response shape lacks result.tools, toolCount becomes undefined and the !toolCount check aborts the batch as if the server advertised zero tools, conflating "genuinely empty" with "unparseable/unexpected body."
    • Fix: Abort only on a confirmed empty tools array; treat a parse/shape failure as its own distinct error rather than folding it into the zero-tools path.
  • packages/hdx-eval/src/harness/preflight.ts:135 -- tools/list is not checked for a top-level JSON-RPC error (unlike initialize at the same layer), so an error response over HTTP 200 is silently reported as "reachable but advertised 0 tools," a misleading diagnostic.
    • Fix: Inspect the parsed tools/list body for error before reading result.tools, and surface it in the failure message.
  • packages/hdx-eval/src/harness/preflight.ts:135 -- The probe issues tools/list immediately after initialize without the notifications/initialized message the MCP lifecycle expects; the current stateless server tolerates this, but a spec-compliant server would reject the call and produce a false abort.
    • Fix: Send notifications/initialized (carrying mcp-session-id when present) between the two requests, and verify against the live server.
  • packages/hdx-eval/src/harness/preflight.ts:30 -- parseJsonRpcBody splits SSE on \n and matches with /^data:\s*(.*)$/; a CRLF-framed stream leaves a trailing \r that the pattern cannot capture (. excludes \r, $ without m matches only end-of-input), so parsing throws — the test only exercises \n framing.
    • Fix: Normalize line endings (strip trailing \r) or split on /\r?\n/, and add a CRLF test case.
  • packages/api/src/mcp/tools/query/builderCatalog.ts:8 -- BUILDER_TOOLS_LIST and SERVER_INSTRUCTIONS embed literal tool/prompt names (including cross-directory clickstack_trace_*) with no compile-time link to their registrations, so a rename silently drifts the steering text.
    • Fix: Add a test asserting every name in BUILDER_TOOLS_LIST resolves to a registered tool.

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

Testing gaps:

  • preflight.ts: plain-JSON (application/json) parse branch, HTTP non-2xx on both requests, JSON-RPC error response, AbortError/timeout, unparseable tools/list body, and mcp-session-id propagation are all unexercised (PR states 8 preflight tests; the diff adds 4).
  • cli.ts: the --no-preflight bypass and the preflight abort/throw path have no coverage.
  • packages/api/src/mcp: no test guards the new server instructions string or the injected builder-vs-SQL steering wording against regression.

The 'prefer builder tools over raw SQL' list was hand-duplicated across
SERVER_INSTRUCTIONS, the clickstack_sql description, and each builder
tool's description, and had already drifted (sql.ts omitted the trace
tools; event_patterns/event_deltas lacked the reciprocal nudge).

Define the builder-tool catalog once in builderCatalog.ts and compose
the server instructions, the sql.ts alternatives list, and every builder
tool's 'prefer me over SQL' nudge from it. Adds event_patterns and
event_deltas nudges that were missing, and a drift-guard test that fails
if the catalog and the registered query/trace tools diverge.

Addresses Greptile review feedback on #2840.
Replace the catalog module's types/functions/lookup (and the drift-guard
test) with three plain constants: BUILDER_TOOLS_LIST, SQL_FALLBACK_CRITERIA,
and a single generic PREFER_BUILDER_OVER_SQL_NUDGE reused by every builder
tool. Same anti-drift guarantee, far less machinery.
Drop redundant coverage (plain-JSON body, non-2xx, JSON-RPC error, and the
preflightMcps ordering wrapper) and use one compact fetch mock helper. Keeps
the four behaviors that matter: happy path w/ tool count, zero-tools failure,
connection-refused actionable message, and stdio passthrough.
@vercel
vercel Bot temporarily deployed to Preview – hyperdx-storybook August 10, 2026 16:29 Inactive
@brandon-pereira
brandon-pereira requested review from a team and fleon and removed request for a team August 10, 2026 16:54
@brandon-pereira
brandon-pereira requested review from a team and teeohhem and removed request for a team and fleon August 10, 2026 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review/tier-2 Low risk — AI review + quick human skim

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant