diff --git a/docs/testing/overview.md b/docs/testing/overview.md index 80f9a4055..501881b5f 100644 --- a/docs/testing/overview.md +++ b/docs/testing/overview.md @@ -43,6 +43,7 @@ Our testing approach prioritizes **functional coverage** over implementation det - Standard library behavior (Promise.all, Array.map, etc.) - Third-party package internals (Zod validation, MSW mocking, etc.) - Implementation details (private methods, internal state) +- Agent/system prompt text or prompt wording (assert behavior via tool inputs/outputs and Sentry API requests instead) - Obvious behavior that will break immediately if wrong ### Example: Context Passing diff --git a/packages/mcp-core/src/tools/catalog/search-events.test.ts b/packages/mcp-core/src/tools/catalog/search-events.test.ts index 8823d7a1d..dbf487f81 100644 --- a/packages/mcp-core/src/tools/catalog/search-events.test.ts +++ b/packages/mcp-core/src/tools/catalog/search-events.test.ts @@ -3,7 +3,6 @@ import { APICallError, generateText } from "ai"; import { HttpResponse, http } from "msw"; import { beforeEach, describe, expect, it, vi } from "vitest"; import { UserInputError } from "../../errors"; -import { MAX_EVENTS_VALIDATION_ATTEMPTS } from "../support/search-events/utils"; import searchEvents from "./search-events"; // Mock the AI SDK @@ -2041,6 +2040,8 @@ describe("search_events", () => { }); it("should repair direct search params with the agent when available", async () => { + let eventsRequestUrl: URL | undefined; + mockGenerateText.mockResolvedValueOnce( mockAIResponse("logs", "severity:error", [ "timestamp", @@ -2053,9 +2054,7 @@ describe("search_events", () => { http.get( "https://sentry.io/api/0/organizations/test-org/events/", ({ request }) => { - const url = new URL(request.url); - expect(url.searchParams.get("dataset")).toBe("logs"); - expect(url.searchParams.get("query")).toBe("severity:error"); + eventsRequestUrl = new URL(request.url); return HttpResponse.json({ data: [ { @@ -2070,7 +2069,7 @@ describe("search_events", () => { ), ); - await searchEvents.handler( + const result = await searchEvents.handler( { organizationSlug: "test-org", regionUrl: null, @@ -2094,10 +2093,17 @@ describe("search_events", () => { }, ); - const prompt = mockGenerateText.mock.calls[0]?.[0]?.prompt; - expect(prompt).toContain("Fix this Sentry event search request"); - expect(prompt).toContain("severity:error"); - expect(prompt).toContain('"dataset": "errors"'); + // Assert behavior via Sentry API inputs + tool output, not agent prompt text. + expect(mockGenerateText).toHaveBeenCalledTimes(1); + expect(eventsRequestUrl).toBeDefined(); + expect(eventsRequestUrl!.searchParams.get("dataset")).toBe("logs"); + expect(eventsRequestUrl!.searchParams.get("query")).toBe("severity:error"); + expect(eventsRequestUrl!.searchParams.getAll("field")).toEqual([ + "timestamp", + "message", + "severity", + ]); + expect(result).toContain("Connection failed to database"); }); it("should handle AI agent errors gracefully", async () => { @@ -2504,10 +2510,11 @@ describe("search_events", () => { expect(result).toContain("Database Error"); }); - it("repairs invalid search params and calls events with updated parameters after validation fails", async () => { - let validateCalls = 0; + it("uses one agent pass to complete incomplete requests before final validation", async () => { let eventsRequestUrl: URL | undefined; + // Incomplete request (no fields/sort) forces the agent path. The agent is + // expected to validateSearch internally; the handler only gates once. mockGenerateText.mockResolvedValueOnce( mockAIResponse( "spans", @@ -2522,47 +2529,7 @@ describe("search_events", () => { mswServer.use( http.get( "https://sentry.io/api/0/organizations/test-org/events/validate/", - () => { - validateCalls += 1; - if (validateCalls === 1) { - return HttpResponse.json({ - valid: false, - projects: [], - dataset: [], - environment: [], - field: [ - { - name: "spon.duration", - valid: false, - attrType: null, - error: "Unknown attribute", - }, - ], - query: { - valid: false, - error: "Invalid syntax", - fields: [ - { - name: "spon.duration", - valid: false, - attrType: null, - error: "Unknown attribute", - }, - ], - }, - orderby: [ - { - name: "-spon.duration", - valid: false, - attrType: null, - error: "Orderby must also be a selected field", - }, - ], - }); - } - - return HttpResponse.json(validEventsValidationResponse); - }, + () => HttpResponse.json(validEventsValidationResponse), ), http.get( "https://sentry.io/api/0/organizations/test-org/events/", @@ -2587,9 +2554,9 @@ describe("search_events", () => { regionUrl: null, projectSlug: null, dataset: "spans", - query: "spon.duration:>100 span.op:db", - fields: ["spon.duration"], - sort: "-spon.duration", + query: "slow database queries", + fields: null, + sort: null, period: "24h", limit: 10, includeExplanation: false, @@ -2605,69 +2572,288 @@ describe("search_events", () => { }, ); - expect(validateCalls).toBe(2); expect(mockGenerateText).toHaveBeenCalledTimes(1); expect(eventsRequestUrl).toBeDefined(); expect(eventsRequestUrl!.searchParams.get("dataset")).toBe("spans"); expect(eventsRequestUrl!.searchParams.get("query")).toBe( "span.duration:>100 span.op:db", ); - expect(eventsRequestUrl!.searchParams.get("query")).not.toContain( - "spon.duration", - ); expect(eventsRequestUrl!.searchParams.getAll("field")).toEqual([ "span.op", "span.duration", ]); - expect(eventsRequestUrl!.searchParams.getAll("field")).not.toContain( - "spon.duration", - ); expect(eventsRequestUrl!.searchParams.get("sort")).toBe("-span.duration"); expect(eventsRequestUrl!.searchParams.get("statsPeriod")).toBe("7d"); expect(result).toContain("span1"); }); - it("repairs invalid query syntax with the agent after validation fails", async () => { + it("fails complete structured requests honestly without an external repair loop", async () => { let validateCalls = 0; - mockGenerateText.mockResolvedValueOnce( - mockAIResponse("spans", "span.op:db", ["span.op", "span.duration"]), - ); - mswServer.use( http.get( "https://sentry.io/api/0/organizations/test-org/events/validate/", () => { validateCalls += 1; - if (validateCalls === 1) { - return HttpResponse.json({ - valid: false, - projects: [], - dataset: [], - environment: [], - field: [], - query: { + return HttpResponse.json({ + valid: false, + projects: [], + dataset: [], + environment: [], + field: [ + { + name: "spon.duration", valid: false, - error: "Invalid syntax", - fields: [], + attrType: null, + error: "Unknown attribute", }, - orderby: [], - }); - } + ], + query: { + valid: false, + error: "Invalid syntax", + fields: [ + { + name: "spon.duration", + valid: false, + attrType: null, + error: "Unknown attribute", + }, + ], + }, + orderby: [], + }); + }, + ), + http.get("https://sentry.io/api/0/organizations/test-org/events/", () => { + throw new Error("searchEvents should not be called"); + }), + ); - return HttpResponse.json(validEventsValidationResponse); + await expect( + searchEvents.handler( + { + organizationSlug: "test-org", + regionUrl: null, + projectSlug: null, + dataset: "spans", + query: "spon.duration:>100", + fields: ["spon.duration"], + sort: "-spon.duration", + period: "24h", + limit: 10, + includeExplanation: false, + }, + { + constraints: { + organizationSlug: null, + regionUrl: null, + projectSlug: null, + }, + accessToken: "test-token", + userId: "1", }, ), - http.get("https://sentry.io/api/0/organizations/test-org/events/", () => - HttpResponse.json({ - data: [ - { - id: "span1", - "span.op": "db", - "span.duration": 42, + ).rejects.toThrow(/Search validation failed/); + + expect(validateCalls).toBe(1); + expect(mockGenerateText).not.toHaveBeenCalled(); + }); + + it("rejects agent rewrites that downgrade structured filters to message full-text", async () => { + // Tweet failure mode: agent rewrites conv_id:X -> message:"*X*". Keep the + // structured filter and fail validation honestly instead of lucky hits. + const validatedQueries: string[] = []; + + mockGenerateText.mockResolvedValue( + mockAIResponse( + "logs", + 'message:"*ZYGC-86ZR*"', + ["timestamp", "message", "trace"], + undefined, + "-timestamp", + ), + ); + + mswServer.use( + http.get( + "https://sentry.io/api/0/organizations/test-org/events/validate/", + ({ request }) => { + const url = new URL(request.url); + validatedQueries.push(url.searchParams.get("query") ?? ""); + return HttpResponse.json({ + valid: false, + projects: [], + dataset: [], + environment: [], + field: [], + query: { + valid: false, + error: null, + fields: [ + { + name: "conv_id", + valid: false, + attrType: null, + error: "Unknown attribute", + }, + ], }, - ], - }), + orderby: [], + }); + }, + ), + http.get("https://sentry.io/api/0/organizations/test-org/events/", () => { + throw new Error("searchEvents should not be called on downgrade"); + }), + ); + + await expect( + searchEvents.handler( + { + organizationSlug: "test-org", + regionUrl: null, + projectSlug: null, + dataset: "logs", + query: "conv_id:ZYGC-86ZR", + fields: null, + sort: null, + period: "24h", + limit: 10, + includeExplanation: false, + }, + { + constraints: { + organizationSlug: null, + regionUrl: null, + projectSlug: null, + }, + accessToken: "test-token", + userId: "1", + }, + ), + ).rejects.toThrow(/Search validation failed/); + + expect(mockGenerateText).toHaveBeenCalledTimes(1); + // Final validation must keep the structured filter, not the message rewrite. + expect(validatedQueries.at(-1)).toBe("conv_id:ZYGC-86ZR"); + expect(validatedQueries.at(-1)).not.toContain("message:"); + }); + + it("rejects partial full-text downgrades when a duplicate structured key remains", async () => { + // Set-based key comparison would miss this: custom remains present after + // custom:foo is dropped into message full-text. + const validatedQueries: string[] = []; + + mockGenerateText.mockResolvedValue( + mockAIResponse( + "logs", + 'custom:bar message:"*foo*"', + ["timestamp", "message", "trace"], + undefined, + "-timestamp", + ), + ); + + mswServer.use( + http.get( + "https://sentry.io/api/0/organizations/test-org/events/validate/", + ({ request }) => { + const url = new URL(request.url); + validatedQueries.push(url.searchParams.get("query") ?? ""); + return HttpResponse.json({ + valid: false, + projects: [], + dataset: [], + environment: [], + field: [], + query: { + valid: false, + error: null, + fields: [ + { + name: "custom", + valid: false, + attrType: null, + error: "Unknown attribute", + }, + ], + }, + orderby: [], + }); + }, + ), + http.get("https://sentry.io/api/0/organizations/test-org/events/", () => { + throw new Error("searchEvents should not be called on downgrade"); + }), + ); + + await expect( + searchEvents.handler( + { + organizationSlug: "test-org", + regionUrl: null, + projectSlug: null, + dataset: "logs", + query: "custom:foo custom:bar", + fields: null, + sort: null, + period: "24h", + limit: 10, + includeExplanation: false, + }, + { + constraints: { + organizationSlug: null, + regionUrl: null, + projectSlug: null, + }, + accessToken: "test-token", + userId: "1", + }, + ), + ).rejects.toThrow(/Search validation failed/); + + expect(mockGenerateText).toHaveBeenCalledTimes(1); + expect(validatedQueries.at(-1)).toBe("custom:foo custom:bar"); + expect(validatedQueries.at(-1)).not.toContain("message:"); + }); + + it("allows real attribute renames that are not full-text downgrades", async () => { + let eventsRequestUrl: URL | undefined; + + // errors is not a trusted structured-trace dataset, so the handler uses the + // downgrade guard only. A real rename must still execute. + mockGenerateText.mockResolvedValueOnce( + mockAIResponse( + "errors", + "error.type:TimeoutError", + ["issue", "title", "error.type", "timestamp"], + undefined, + "-timestamp", + ), + ); + + mswServer.use( + http.get( + "https://sentry.io/api/0/organizations/test-org/events/validate/", + () => HttpResponse.json(validEventsValidationResponse), + ), + http.get( + "https://sentry.io/api/0/organizations/test-org/events/", + ({ request }) => { + eventsRequestUrl = new URL(request.url); + return HttpResponse.json({ + data: [ + { + id: "err1", + issue: "PROJ-1", + title: "TimeoutError", + "error.type": "TimeoutError", + timestamp: "2024-01-15T10:30:00Z", + }, + ], + }); + }, ), ); @@ -2676,10 +2862,10 @@ describe("search_events", () => { organizationSlug: "test-org", regionUrl: null, projectSlug: null, - dataset: "spans", - query: "span.op:db AND", - fields: ["span.duration"], - sort: "-span.duration", + dataset: "errors", + query: "eror.type:TimeoutError", + fields: null, + sort: null, period: "24h", limit: 10, includeExplanation: false, @@ -2695,13 +2881,22 @@ describe("search_events", () => { }, ); - expect(validateCalls).toBe(2); expect(mockGenerateText).toHaveBeenCalledTimes(1); - expect(result).toContain("span1"); + expect(eventsRequestUrl).toBeDefined(); + expect(eventsRequestUrl!.searchParams.get("dataset")).toBe("errors"); + expect(eventsRequestUrl!.searchParams.get("query")).toBe( + "error.type:TimeoutError", + ); + expect(eventsRequestUrl!.searchParams.getAll("field")).toEqual([ + "issue", + "title", + "error.type", + "timestamp", + ]); + expect(result).toContain("TimeoutError"); }); - it("keeps prior fields when validation repair returns an empty fields array", async () => { - let validateCalls = 0; + it("keeps caller fields when the agent returns an empty fields array", async () => { let eventsRequestUrl: URL | undefined; mockGenerateText.mockResolvedValueOnce( @@ -2717,33 +2912,7 @@ describe("search_events", () => { mswServer.use( http.get( "https://sentry.io/api/0/organizations/test-org/events/validate/", - () => { - validateCalls += 1; - if (validateCalls === 1) { - return HttpResponse.json({ - valid: false, - projects: [], - dataset: [], - environment: [], - field: [ - { - name: "spon.duration", - valid: false, - attrType: null, - error: "Unknown attribute", - }, - ], - query: { - valid: false, - error: "Invalid syntax", - fields: [], - }, - orderby: [], - }); - } - - return HttpResponse.json(validEventsValidationResponse); - }, + () => HttpResponse.json(validEventsValidationResponse), ), http.get( "https://sentry.io/api/0/organizations/test-org/events/", @@ -2762,9 +2931,9 @@ describe("search_events", () => { regionUrl: null, projectSlug: null, dataset: "spans", - query: "spon.duration:>100", + query: "span.duration:>100", fields: ["span.duration"], - sort: "-span.duration", + sort: null, period: "24h", limit: 10, includeExplanation: false, @@ -2786,8 +2955,7 @@ describe("search_events", () => { ]); }); - it("merges repaired environment into the events search query for non-replay datasets", async () => { - let validateCalls = 0; + it("merges agent environment into the events search query for non-replay datasets", async () => { let eventsRequestUrl: URL | undefined; mockGenerateText.mockResolvedValueOnce( @@ -2805,26 +2973,7 @@ describe("search_events", () => { mswServer.use( http.get( "https://sentry.io/api/0/organizations/test-org/events/validate/", - () => { - validateCalls += 1; - if (validateCalls === 1) { - return HttpResponse.json({ - valid: false, - projects: [], - dataset: [], - environment: [], - field: [], - query: { - valid: false, - error: "Invalid syntax", - fields: [], - }, - orderby: [], - }); - } - - return HttpResponse.json(validEventsValidationResponse); - }, + () => HttpResponse.json(validEventsValidationResponse), ), http.get( "https://sentry.io/api/0/organizations/test-org/events/", @@ -2843,9 +2992,9 @@ describe("search_events", () => { regionUrl: null, projectSlug: null, dataset: "spans", - query: "spon.duration:>100", - fields: ["span.duration"], - sort: "-span.duration", + query: "span.duration:>100", + fields: null, + sort: null, period: "24h", limit: 10, includeExplanation: false, @@ -2867,7 +3016,7 @@ describe("search_events", () => { ); }); - it("rejects search after MAX_EVENTS_VALIDATION_ATTEMPTS without calling events", async () => { + it("does not call events when final validation fails", async () => { let validateCalls = 0; mockGenerateText.mockResolvedValue( @@ -2914,8 +3063,8 @@ describe("search_events", () => { projectSlug: null, dataset: "spans", query: "tags[missing]:true", - fields: ["span.duration"], - sort: "-span.duration", + fields: null, + sort: null, period: "24h", limit: 10, includeExplanation: false, @@ -2930,12 +3079,10 @@ describe("search_events", () => { userId: "1", }, ), - ).rejects.toThrow(/Search validation failed after repair attempts/); + ).rejects.toThrow(/Search validation failed/); - expect(validateCalls).toBe(1 + MAX_EVENTS_VALIDATION_ATTEMPTS); - expect(mockGenerateText).toHaveBeenCalledTimes( - MAX_EVENTS_VALIDATION_ATTEMPTS, - ); + expect(validateCalls).toBe(1); + expect(mockGenerateText).toHaveBeenCalledTimes(1); }); it("rejects search immediately when validation fails without an agent provider", async () => { diff --git a/packages/mcp-core/src/tools/catalog/search-events.ts b/packages/mcp-core/src/tools/catalog/search-events.ts index 66024dd49..8d3d41c1e 100644 --- a/packages/mcp-core/src/tools/catalog/search-events.ts +++ b/packages/mcp-core/src/tools/catalog/search-events.ts @@ -1,4 +1,4 @@ -import { getActiveSpan, setTag, startSpan } from "@sentry/core"; +import { getActiveSpan, setTag } from "@sentry/core"; import { z } from "zod"; import { UserInputError } from "../../errors"; import { hasAgentProvider } from "../../internal/agents/provider-factory"; @@ -43,8 +43,8 @@ import { import { formatEventsValidationResults, isAggregateQuery, + isSemanticFilterDowngrade, looksLikeSentrySearchSyntax, - MAX_EVENTS_VALIDATION_ATTEMPTS, recordEventsSearchValidationTelemetry, validateEventsSearch, } from "../support/search-events/utils"; @@ -54,15 +54,6 @@ const DEFAULT_EVENTS_SORT = "-timestamp"; type SearchEventsAgentResult = z.output; -type EventsSearchState = { - dataset: PublicEventsDataset; - sentryQuery: string; - fields: string[]; - sortParam: string; - environment?: string | string[] | null; - timeParams: { statsPeriod?: string; start?: string; end?: string }; -}; - function defaultSortForDataset(dataset: PublicEventsDataset | "replays") { return dataset === "replays" ? DEFAULT_REPLAY_SORT : DEFAULT_EVENTS_SORT; } @@ -137,23 +128,6 @@ function buildRequestFields( : fields; } -function applyValidationRepairFromAgent( - state: EventsSearchState, - parsed: SearchEventsAgentResult, -): EventsSearchState { - const sortParam = parsed.sort.trim(); - const repairedFields = - parsed.fields && parsed.fields.length > 0 ? parsed.fields : state.fields; - return { - dataset: parsed.dataset === "replays" ? state.dataset : parsed.dataset, - sentryQuery: parsed.query || state.sentryQuery, - fields: augmentFieldsWithSort(repairedFields, sortParam), - sortParam, - environment: parsed.environment ? parsed.environment : state.environment, - timeParams: parseAgentTimeRange(parsed.timeRange) ?? state.timeParams, - }; -} - function isTraceItemDataset(dataset: PublicEventsDataset | "replays"): boolean { return dataset === "spans" || dataset === "logs" || dataset === "metrics"; } @@ -281,6 +255,10 @@ function choosePreservingRepairedQuery(params: { return appendSearchFilter(originalQuery, params.filter); } + if (isSemanticFilterDowngrade(originalQuery, repairedQuery)) { + return appendSearchFilter(originalQuery, params.filter); + } + if (!originalQuery || preservesSearchTokens(originalQuery, repairedQuery)) { return appendSearchFilter(repairedQuery, params.filter); } @@ -288,7 +266,7 @@ function choosePreservingRepairedQuery(params: { return appendSearchFilter(originalQuery, params.filter); } -function buildSearchRepairPrompt(params: { +function buildAgentPrompt(params: { query?: string; dataset: PublicEventsDataset | "replays"; fields?: string[] | null; @@ -297,12 +275,14 @@ function buildSearchRepairPrompt(params: { environment?: string | string[] | null; }): string { return [ - "Fix this Sentry event search request.", + "Translate this Sentry event search request.", "The query may be natural language or already-valid Sentry search syntax.", "Preserve valid explicit parameters, but correct dataset, query syntax, fields, sort, and time range when they conflict or would fail.", - "If the user query already uses Sentry search syntax, treat its filters as authoritative unless the search validation step proves a field is invalid.", + "If the user query already uses Sentry search syntax, treat its filters as authoritative unless validateSearch proves a field is invalid.", + "Never replace a structured field filter with message/log.body/full-text matching. If no valid attribute exists for an explicit field:value filter, keep the field and let validation fail.", "For spans, logs, and metrics, use datasetAttributes to discover likely fields with substringMatch, query, and attributeTypes before dropping or renaming explicit fields.", "A broad datasetAttributes result may be truncated, so absence from that preview does not prove an explicit field is invalid.", + "For non-replay datasets, call validateSearch on the candidate request and fix failures in this same pass before returning.", "For non-replay datasets, convert environment parameters into query filters. For replays, keep environment in the separate environment parameter.", "", `User query: ${params.query || "(empty)"}`, @@ -321,44 +301,6 @@ function buildSearchRepairPrompt(params: { ].join("\n"); } -function buildValidationFailureRepairPrompt(params: { - originalQuery?: string; - dataset: PublicEventsDataset | "replays"; - query: string; - fields: string[]; - sort: string; - environment?: string | string[] | null; - timeParams: { statsPeriod?: string; start?: string; end?: string }; - validation: string; -}): string { - return [ - "Sentry events search validation failed. Repair the request so it passes validation.", - "Use the validation results to correct every invalid parameter: dataset, environment, query filters, selected fields, sort, and time range.", - "Use datasetAttributes to discover valid field names before renaming or replacing invalid fields.", - "Prefer renaming invalid fields to valid attributes instead of dropping them.", - "Ensure the sort field is included in the fields array.", - "For non-replay datasets, convert environment filters into query syntax when validation rejects the environment parameter.", - "For replays, keep environment in the separate environment parameter.", - "", - params.validation.trim(), - "", - `Original user query: ${params.originalQuery || "(empty)"}`, - "Current request:", - JSON.stringify( - { - dataset: params.dataset, - query: params.query, - fields: params.fields, - sort: params.sort, - environment: params.environment ?? null, - ...params.timeParams, - }, - null, - 2, - ), - ].join("\n"); -} - export default defineTool({ name: "search_events", skills: ["inspect", "triage", "seer"], // Available in inspect, triage, and seer skills @@ -538,7 +480,7 @@ export default defineTool({ run: async () => ( await searchEventsAgent({ - query: buildSearchRepairPrompt({ + query: buildAgentPrompt({ query: params.query, dataset: inputDataset, fields: params.fields, @@ -574,7 +516,10 @@ export default defineTool({ repairedQuery: parsed.query, filter: environmentFilter, }) - : parsed.query || ""; + : looksLikeSentrySearchSyntax(params.query) && + isSemanticFilterDowngrade(params.query ?? "", parsed.query || "") + ? (params.query ?? "") + : parsed.query || ""; sortParam = shouldTrustExplicitSearchParams && explicitSort ? explicitSort @@ -694,13 +639,14 @@ export default defineTool({ const requestFields = buildRequestFields(dataset, fields); - let validationExplanation: string | undefined; + // Final gate only. The agent should already have used validateSearch while + // constructing the request; the handler does not run a second repair agent. sentryQuery = applyEnvironmentToEventsQuery( dataset, sentryQuery, environment, ); - let lastValidation = await validateEventsSearch(apiService, { + const lastValidation = await validateEventsSearch(apiService, { organizationSlug, dataset, fields: requestFields, @@ -716,137 +662,12 @@ export default defineTool({ }); if (!lastValidation.valid) { - if (!hasAgentProvider()) { - const formatted = formatEventsValidationResults(lastValidation); - throw new UserInputError( - formatted - ? `Search validation failed:\n${formatted}` - : "Search validation failed.", - ); - } - - let providerUnavailableDuringValidationRepair = false; - for ( - let attempt = 0; - attempt < MAX_EVENTS_VALIDATION_ATTEMPTS && !lastValidation.valid; - attempt++ - ) { - await startSpan( - { - name: "search_events.validation_repair", - op: "gen_ai.embedded_agent", - attributes: { - "app.search_events.validation.repair_iteration": attempt + 1, - }, - }, - async () => { - const parsed = await withProviderFallback({ - operation: "search_events.validation_repair", - fallback: () => ({ - dataset, - query: sentryQuery, - fields, - sort: sortParam, - environment: environment ?? null, - timeRange: - "statsPeriod" in timeParams - ? { statsPeriod: timeParams.statsPeriod ?? "14d" } - : { - start: timeParams.start ?? "", - end: timeParams.end ?? "", - }, - explanation: "", - }), - onFallback: () => { - providerUnavailableDuringValidationRepair = true; - }, - run: async () => - ( - await searchEventsAgent({ - query: buildValidationFailureRepairPrompt({ - originalQuery: params.query, - dataset, - query: sentryQuery, - fields, - sort: sortParam, - environment, - timeParams, - validation: formatEventsValidationResults(lastValidation), - }), - organizationSlug, - apiService, - projectId, - }) - ).result, - }); - if (!parsed.sort?.trim()) { - throw new UserInputError( - `Search validation repair failed: agent response missing required 'sort' parameter. Received: ${JSON.stringify(parsed, null, 2)}.`, - ); - } - - ({ - dataset, - sentryQuery, - fields, - sortParam, - environment, - timeParams, - } = applyValidationRepairFromAgent( - { - dataset: dataset as PublicEventsDataset, - sentryQuery, - fields, - sortParam, - environment, - timeParams, - }, - parsed, - )); - validationExplanation = parsed.explanation || validationExplanation; - sentryQuery = applyEnvironmentToEventsQuery( - dataset, - sentryQuery, - environment, - ); - - lastValidation = await validateEventsSearch(apiService, { - organizationSlug, - dataset, - fields: buildRequestFields(dataset, fields), - query: sentryQuery, - sort: sortParam, - projectId, - environment: environment ?? undefined, - ...timeParams, - }); - recordEventsSearchValidationTelemetry({ - attempt: attempt + 1, - repairIteration: attempt + 1, - validation: lastValidation, - }); - }, - ); - - if (providerUnavailableDuringValidationRepair) { - break; - } - } - - if (!lastValidation.valid && !providerUnavailableDuringValidationRepair) { - const formatted = formatEventsValidationResults(lastValidation); - throw new UserInputError( - formatted - ? `Search validation failed after repair attempts:\n${formatted}` - : "Search validation failed after repair attempts.", - ); - } - - if (validationExplanation) { - explanation = explanation - ? `${explanation} ${validationExplanation}` - : validationExplanation; - } + const formatted = formatEventsValidationResults(lastValidation); + throw new UserInputError( + formatted + ? `Search validation failed:\n${formatted}` + : "Search validation failed.", + ); } const finalRequestFields = buildRequestFields(dataset, fields); diff --git a/packages/mcp-core/src/tools/support/search-events/agent.ts b/packages/mcp-core/src/tools/support/search-events/agent.ts index 15acc7d7b..70a41cf1e 100644 --- a/packages/mcp-core/src/tools/support/search-events/agent.ts +++ b/packages/mcp-core/src/tools/support/search-events/agent.ts @@ -1,12 +1,15 @@ import { z } from "zod"; -import { callEmbeddedAgent } from "../../../internal/agents/callEmbeddedAgent"; import type { SentryApiService } from "../../../api-client"; -import { createOtelLookupTool } from "../../../internal/agents/tools/otel-semantics"; +import { callEmbeddedAgent } from "../../../internal/agents/callEmbeddedAgent"; import { createDatasetFieldsTool } from "../../../internal/agents/tools/dataset-fields"; +import { createOtelLookupTool } from "../../../internal/agents/tools/otel-semantics"; import { createWhoamiTool } from "../../../internal/agents/tools/whoami"; -import { createDatasetAttributesTool } from "./utils"; -import { systemPrompt } from "./config"; import { PUBLIC_EVENTS_DATASETS } from "../../../utils/events-datasets"; +import { systemPrompt } from "./config"; +import { + createDatasetAttributesTool, + createValidateEventsSearchTool, +} from "./utils"; const SEARCH_EVENTS_DATASETS = [...PUBLIC_EVENTS_DATASETS, "replays"] as const; @@ -100,6 +103,11 @@ export async function searchEventsAgent( organizationSlug: options.organizationSlug, projectId: options.projectId, }); + const validateSearchTool = createValidateEventsSearchTool({ + apiService: options.apiService, + organizationSlug: options.organizationSlug, + projectId: options.projectId, + }); const otelLookupTool = createOtelLookupTool({ apiService: options.apiService, organizationSlug: options.organizationSlug, @@ -122,6 +130,7 @@ export async function searchEventsAgent( prompt: options.query, tools: { datasetAttributes: datasetAttributesTool, + validateSearch: validateSearchTool, replayFields: replayFieldsTool, otelSemantics: otelLookupTool, whoami: whoamiTool, diff --git a/packages/mcp-core/src/tools/support/search-events/config.ts b/packages/mcp-core/src/tools/support/search-events/config.ts index 8f137e1e5..2a9d6823d 100644 --- a/packages/mcp-core/src/tools/support/search-events/config.ts +++ b/packages/mcp-core/src/tools/support/search-events/config.ts @@ -5,6 +5,7 @@ export const systemPrompt = `You are a Sentry query translator. You need to: 3. Use the otelSemantics tool if you need OpenTelemetry semantic conventions 4. Convert the natural language query to Sentry's search syntax (NOT SQL syntax) 5. Decide which fields to return in the results +6. For non-replay datasets, call validateSearch on the candidate request and fix failures before returning CRITICAL: Sentry does NOT use SQL syntax. Do NOT generate SQL-like queries. @@ -40,6 +41,8 @@ TOOL USAGE GUIDELINES: 5. IMPORTANT: For ambiguous terms like "user agents", "browser", "client" - use the appropriate field discovery tool instead of guessing field names 6. When the user already supplied Sentry search syntax for spans/logs/metrics, call datasetAttributes with substringMatch or query filters from the request before dropping or renaming fields 7. Use datasetAttributes substringMatch, query, and attributeTypes for targeted lookup when broad field discovery is truncated +8. For non-replay datasets, call validateSearch after constructing the candidate request. If invalid, fix and validate again in this same pass +9. NEVER replace a structured field:value filter with message/log.body/full-text matching. If an explicit field is unavailable on the dataset, keep it and let validation fail instead of inventing a weaker query CRITICAL - TOOL RESPONSE HANDLING: All tools return responses in this format: {error?: string, result?: data} @@ -236,6 +239,7 @@ PROCESS: 3. Use datasetAttributes or replayFields to discover available fields 4. Use otelSemantics tool if needed for OpenTelemetry attributes 5. Construct the final query with proper fields, sort parameters, and replay environment when needed +6. For non-replay datasets, validateSearch the candidate and only return after it passes (or after you cannot fix it without weakening structured filters) COMMON ERRORS TO AVOID: - Using SQL syntax (IS NOT NULL, IS NULL, yesterday(), today(), etc.) - Use has: operator and timeRange instead diff --git a/packages/mcp-core/src/tools/support/search-events/utils.test.ts b/packages/mcp-core/src/tools/support/search-events/utils.test.ts index 47a4c8270..d2bca4fa1 100644 --- a/packages/mcp-core/src/tools/support/search-events/utils.test.ts +++ b/packages/mcp-core/src/tools/support/search-events/utils.test.ts @@ -1,15 +1,16 @@ -import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; -import { http, HttpResponse } from "msw"; import { mswServer } from "@sentry/mcp-server-mocks"; +import { HttpResponse, http } from "msw"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { SentryApiService } from "../../../api-client"; +import * as logging from "../../../telem/logging"; import { fetchCustomAttributes, - formatEventValue, formatEventsValidationResults, + formatEventValue, formatKnownUserValue, + isSemanticFilterDowngrade, looksLikeSentrySearchSyntax, } from "./utils"; -import { SentryApiService } from "../../../api-client"; -import * as logging from "../../../telem/logging"; describe("formatEventValue", () => { describe("primitives", () => { @@ -221,6 +222,104 @@ describe("search query helpers", () => { expect(looksLikeSentrySearchSyntax("Note: show slow spans")).toBe(false); expect(looksLikeSentrySearchSyntax("ERROR: service is down")).toBe(false); }); + + it("detects message full-text downgrades but allows real field renames", () => { + expect( + isSemanticFilterDowngrade("conv_id:ZYGC-86ZR", 'message:"*ZYGC-86ZR*"'), + ).toBe(true); + expect( + isSemanticFilterDowngrade( + "conv_id:ZYGC-86ZR", + "message:*ZYGC-86ZR* environment:prod", + ), + ).toBe(true); + + // Legitimate typo/rename repairs must not be treated as downgrades. + expect( + isSemanticFilterDowngrade( + "spon.duration:>100 span.op:db", + "span.duration:>100 span.op:db", + ), + ).toBe(false); + expect( + isSemanticFilterDowngrade( + "tags[type]:Unified", + "tags[type]:Unified has:span.status", + ), + ).toBe(false); + expect(isSemanticFilterDowngrade("span.op:db AND", "span.op:db")).toBe( + false, + ); + + // Natural language input is not a structured-filter downgrade case. + expect( + isSemanticFilterDowngrade( + "errors mentioning ZYGC-86ZR", + 'message:"*ZYGC-86ZR*"', + ), + ).toBe(false); + + // message: inside a quoted value is not a full-text filter. + expect( + isSemanticFilterDowngrade( + "custom:hello", + 'transaction:"handle message:hello"', + ), + ).toBe(false); + + // Duplicate structured keys must still catch a partial full-text downgrade. + // Set-based key comparison would miss this because "custom" remains present. + expect( + isSemanticFilterDowngrade( + "custom:foo custom:bar", + 'custom:bar message:"*foo*"', + ), + ).toBe(true); + expect( + isSemanticFilterDowngrade( + "custom:foo custom:foo", + 'custom:foo message:"*foo*"', + ), + ).toBe(true); + + // Keeping both structured values (or renaming one) is not a downgrade. + expect( + isSemanticFilterDowngrade( + "custom:foo custom:bar", + "custom:foo custom:bar environment:prod", + ), + ).toBe(false); + expect( + isSemanticFilterDowngrade( + "custom:foo custom:bar", + "tags[custom]:foo custom:bar", + ), + ).toBe(false); + + // Quoted multi-word values must keep later words for downgrade detection. + // Truncating at the first space would miss message rewrites of "world". + expect( + isSemanticFilterDowngrade( + 'transaction:"hello world"', + 'message:"*world*"', + ), + ).toBe(true); + expect( + isSemanticFilterDowngrade( + "transaction:'hello world'", + 'log.body:"*hello world*"', + ), + ).toBe(true); + + // Bare substring matches are not enough — short values must not false-hit + // inside unrelated full-text (e.g. "1" inside "401"). + expect( + isSemanticFilterDowngrade("id:1", 'message:"error 401"'), + ).toBe(false); + expect( + isSemanticFilterDowngrade("id:1", 'message:"error 1"'), + ).toBe(true); + }); }); describe("fetchCustomAttributes", () => { diff --git a/packages/mcp-core/src/tools/support/search-events/utils.ts b/packages/mcp-core/src/tools/support/search-events/utils.ts index f07ece943..79028959a 100644 --- a/packages/mcp-core/src/tools/support/search-events/utils.ts +++ b/packages/mcp-core/src/tools/support/search-events/utils.ts @@ -1,13 +1,13 @@ -import { z } from "zod"; import { getActiveSpan } from "@sentry/core"; -import { UserInputError } from "../../../errors"; +import { z } from "zod"; import type { - SentryApiService, - TraceItemAttributeType, EventsQueryValidation, EventsValidationResult, + SentryApiService, + TraceItemAttributeType, TraceItemType, } from "../../../api-client"; +import { UserInputError } from "../../../errors"; import { agentTool, recordAgentToolResultCount, @@ -18,8 +18,8 @@ import { } from "../../../internal/user-formatting"; import { type EventsDataset, - PUBLIC_EVENTS_DATASETS, normalizeEventsDataset, + PUBLIC_EVENTS_DATASETS, } from "../../../utils/events-datasets"; // Type for flexible event data that can contain any fields @@ -116,6 +116,251 @@ export function looksLikeSentrySearchSyntax(query?: string): boolean { return false; } +const FULL_TEXT_SEARCH_KEYS = new Set(["message", "log.body"]); + +/** + * Replace quoted regions with same-length placeholders so colons inside quotes + * are not treated as filter-key separators (e.g. transaction:"handle message:hello"). + * Length is preserved so offsets still map back to the original query. + */ +function maskQuotedRegions(query: string): string { + let out = ""; + let quote: '"' | "'" | null = null; + let escaped = false; + + for (const char of query) { + if (escaped) { + escaped = false; + out += quote ? "x" : char; + continue; + } + if (char === "\\") { + escaped = true; + out += quote ? "x" : char; + continue; + } + if (quote) { + if (char === quote) { + quote = null; + out += char; + } else { + // Keep length; neutralize token separators inside quotes. + out += char === ":" || /\s/.test(char) ? "x" : char; + } + continue; + } + if (char === '"' || char === "'") { + quote = char; + out += char; + continue; + } + out += char; + } + + return out; +} + +type SearchFilterOccurrence = { + key: string; + value: string; +}; + +function normalizeFilterValue(rawValue: string): string { + return rawValue + .replace(/^['"]|['"]$/g, "") + .replace(/^\*+|\*+$/g, "") + .trim() + .toLowerCase(); +} + +/** + * Read one filter value starting at `valueStart` in the original query. + * Quoted values keep interior whitespace; unquoted values stop at whitespace. + */ +function readRawFilterValue(query: string, valueStart: number): string | undefined { + if (valueStart >= query.length) { + return undefined; + } + + const first = query[valueStart]; + if (first === '"' || first === "'") { + let escaped = false; + for (let i = valueStart + 1; i < query.length; i += 1) { + const char = query[i]; + if (escaped) { + escaped = false; + continue; + } + if (char === "\\") { + escaped = true; + continue; + } + if (char === first) { + return query.slice(valueStart, i + 1); + } + } + // Unclosed quote: take the remainder so comparison still sees later words. + return query.slice(valueStart); + } + + const valueMatch = query.slice(valueStart).match(/^\S+/); + return valueMatch?.[0]; +} + +/** + * Extract every field:value occurrence while preserving multiplicity and quote + * boundaries. Values are normalized for comparison (strip wrapping quotes and + * leading/trailing wildcards). + */ +function searchFilterOccurrences(query: string): SearchFilterOccurrence[] { + const occurrences: SearchFilterOccurrence[] = []; + const masked = maskQuotedRegions(query); + + for (const match of masked.matchAll(SENTRY_SEARCH_TOKEN_PATTERN)) { + const key = match[2]?.toLowerCase(); + if (!key || match.index === undefined) { + continue; + } + + // Read the real value from the original query at the same offset so quotes + // and multi-word quoted values are preserved before normalization. + const valueStart = match.index + match[0].indexOf(":") + 1; + const rawValue = readRawFilterValue(query, valueStart); + if (!rawValue) { + continue; + } + + const value = normalizeFilterValue(rawValue); + if (!value) { + continue; + } + + occurrences.push({ key, value }); + } + + return occurrences; +} + +function structuredFilterOccurrences( + query: string, +): SearchFilterOccurrence[] { + return searchFilterOccurrences(query).filter( + (occurrence) => !FULL_TEXT_SEARCH_KEYS.has(occurrence.key), + ); +} + +function fullTextFilterValues(query: string): string[] { + return searchFilterOccurrences(query) + .filter((occurrence) => FULL_TEXT_SEARCH_KEYS.has(occurrence.key)) + .map((occurrence) => occurrence.value); +} + +function filterOccurrenceIdentity(occurrence: SearchFilterOccurrence): string { + return `${occurrence.key}\0${occurrence.value}`; +} + +function escapeRegExp(value: string): string { + return value.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); +} + +/** + * True when `needle` appears in `haystack` as a whole token, not a bare + * substring. Prevents `id:1` from matching inside `message:"error 401"`. + */ +function containsAsWholeToken(haystack: string, needle: string): boolean { + if (!haystack || !needle) { + return false; + } + if (haystack === needle) { + return true; + } + + const escaped = escapeRegExp(needle); + // Token edges are start/end or a non-alphanumeric/underscore char so short + // numeric fragments cannot match inside longer numbers. + return new RegExp(`(?:^|[^A-Za-z0-9_])${escaped}(?:$|[^A-Za-z0-9_])`).test( + haystack, + ); +} + +function isRelatedFilterValue(left: string, right: string): boolean { + return ( + containsAsWholeToken(left, right) || containsAsWholeToken(right, left) + ); +} + +/** + * Structured filters present in `original` that are not covered by multiset + * cardinality in `repaired` (same key+value pair counts). + */ +function unmatchedStructuredFilters( + original: SearchFilterOccurrence[], + repaired: SearchFilterOccurrence[], +): SearchFilterOccurrence[] { + const remaining = new Map(); + for (const occurrence of repaired) { + const identity = filterOccurrenceIdentity(occurrence); + remaining.set(identity, (remaining.get(identity) ?? 0) + 1); + } + + const unmatched: SearchFilterOccurrence[] = []; + for (const occurrence of original) { + const identity = filterOccurrenceIdentity(occurrence); + const count = remaining.get(identity) ?? 0; + if (count > 0) { + remaining.set(identity, count - 1); + continue; + } + unmatched.push(occurrence); + } + + return unmatched; +} + +/** + * True when a structured field:value filter was replaced with message/log.body + * full-text matching (false-success path). Allows real attribute renames. + * + * Uses multiset key+value matching so dropping one of several identical keys + * (e.g. `custom:foo custom:bar` → `custom:bar message:"*foo*"`) is still caught. + * Value comparison is whole-token, not bare substring, so unrelated full-text + * (e.g. `id:1` vs `message:"error 401"`) is not treated as a downgrade. + */ +export function isSemanticFilterDowngrade( + originalQuery: string, + repairedQuery: string, +): boolean { + if (!looksLikeSentrySearchSyntax(originalQuery)) { + return false; + } + + const originalFilters = structuredFilterOccurrences(originalQuery); + if (originalFilters.length === 0) { + return false; + } + + const repairedFilters = structuredFilterOccurrences(repairedQuery); + const droppedFilters = unmatchedStructuredFilters( + originalFilters, + repairedFilters, + ); + if (droppedFilters.length === 0) { + return false; + } + + const repairedFullTextValues = fullTextFilterValues(repairedQuery); + if (repairedFullTextValues.length === 0) { + // Renames keep values on non-full-text attributes and do not need this guard. + return false; + } + + return droppedFilters.some((filter) => + repairedFullTextValues.some((fullTextValue) => + isRelatedFilterValue(filter.value, fullTextValue), + ), + ); +} + function isPrimitive( value: unknown, ): value is string | number | boolean | null { @@ -606,8 +851,6 @@ export function recordEventsSearchValidationTelemetry({ } } -export const MAX_EVENTS_VALIDATION_ATTEMPTS = 3; - export async function validateEventsSearch( apiService: SentryApiService, { @@ -819,3 +1062,90 @@ Use these examples as patterns for constructing your query.`; }, }); } + +/** + * Create a tool for the agent to validate a candidate events search before returning. + * Prefer this over external try/fail/retry orchestration in the tool handler. + */ +export function createValidateEventsSearchTool(options: { + apiService: SentryApiService; + organizationSlug: string; + projectId?: string; +}) { + const { apiService, organizationSlug, projectId } = options; + + return agentTool({ + description: + "Validate a candidate Sentry events search before returning it. Call this after constructing dataset/query/fields/sort. If invalid, fix the request and validate again. Never replace a structured field:value filter with message/log.body full-text matching.", + parameters: z.object({ + dataset: z + .enum(PUBLIC_EVENTS_DATASETS) + .describe("Dataset for the candidate search"), + query: z + .string() + .describe("Candidate Sentry search query string (may be empty)"), + fields: z + .array(z.string()) + .min(1) + .describe( + "Candidate fields, including any aggregate functions and sort field", + ), + sort: z.string().min(1).describe("Candidate sort parameter"), + statsPeriod: z + .string() + .optional() + .describe("Optional relative time period like 1h, 24h, 7d"), + start: z.string().optional().describe("Optional ISO 8601 start time"), + end: z.string().optional().describe("Optional ISO 8601 end time"), + environment: z + .union([z.string().min(1), z.array(z.string().min(1)).min(1)]) + .optional() + .describe( + "Optional environment filter. Prefer query filters for non-replay datasets.", + ), + }), + execute: async ({ + dataset, + query, + fields, + sort, + statsPeriod, + start, + end, + environment, + }) => { + const validation = await validateEventsSearch(apiService, { + organizationSlug, + dataset, + fields, + query, + sort, + projectId, + environment, + statsPeriod, + start, + end, + }); + + recordEventsSearchValidationTelemetry({ + attempt: 0, + validation, + }); + + if (validation.valid) { + return { + valid: true, + message: "Search validation passed.", + }; + } + + const formatted = formatEventsValidationResults(validation); + return { + valid: false, + message: formatted + ? `Search validation failed:\n${formatted}` + : "Search validation failed.", + }; + }, + }); +} diff --git a/packages/mcp-server-evals/src/evals/search-events-agent.eval.ts b/packages/mcp-server-evals/src/evals/search-events-agent.eval.ts index 9ca562017..6d730bfbb 100644 --- a/packages/mcp-server-evals/src/evals/search-events-agent.eval.ts +++ b/packages/mcp-server-evals/src/evals/search-events-agent.eval.ts @@ -134,6 +134,60 @@ describeEval("search-events-agent", { timeRange: { statsPeriod: "7d" }, }, }, + { + // Structured filters must stay structured. Never rewrite a field:value + // filter into message/log.body full-text (tweet false-success path). + input: + "In logs, search for conv_id:ZYGC-86ZR from the last 24 hours. Return timestamp, message, and trace.", + expectedTools: [ + { + name: "datasetAttributes", + arguments: { + dataset: "logs", + }, + }, + ], + expected: { + dataset: "logs", + query: (value: unknown) => + typeof value === "string" && + value.includes("conv_id:ZYGC-86ZR") && + !/\bmessage\s*:/i.test(value) && + !/\blog\.body\s*:/i.test(value), + fields: (value: unknown) => + Array.isArray(value) && + ["timestamp", "message", "trace"].every((field) => + value.includes(field), + ), + sort: "-timestamp", + timeRange: { statsPeriod: "24h" }, + }, + }, + { + // Real attribute renames are still allowed when the structured value + // stays on a non-full-text field. + input: + "In spans, search for spon.duration:>100 over the last day. Return span.duration sorted descending.", + expectedTools: [ + { + name: "datasetAttributes", + arguments: { + dataset: "spans", + }, + }, + ], + expected: { + dataset: "spans", + query: (value: unknown) => + typeof value === "string" && + value.includes("span.duration:>100") && + !/\bmessage\s*:/i.test(value), + fields: (value: unknown) => + Array.isArray(value) && value.includes("span.duration"), + sort: "-span.duration", + timeRange: { statsPeriod: "24h" }, + }, + }, { // Query requiring equation field calculation input: "How many total tokens did we consume yesterday",