diff --git a/apps/ai/scripts/pr-review-local.ts b/apps/ai/scripts/pr-review-local.ts index f8b8663195..968c2d6598 100644 --- a/apps/ai/scripts/pr-review-local.ts +++ b/apps/ai/scripts/pr-review-local.ts @@ -58,7 +58,7 @@ import { AGENTS } from "@/chat/agents" import type { ChatTurnEvent } from "@/chat/events" import { withToolTranscript } from "@/chat/close-out" import { PR_REVIEW_TOOLS } from "@/chat/permissions" -import { normalizeArguments, argumentNotices } from "@/mcp/lib/decode-issues" +import { normalizeArguments, argumentWarnings } from "@/mcp/lib/decode-issues" import { PR_REVIEW_CLOSE_OUT_PROMPT } from "@/chat/prompts" import { makeReviewCoverage } from "@/chat/review-coverage" import { makeReviewLedger } from "@/chat/review-ledger" @@ -766,9 +766,9 @@ export const makeExecutor = (input: { return { ...result, content: [ - ...argumentNotices(normalized, name).map((notice) => ({ + ...argumentWarnings(normalized, name).map((warning) => ({ type: "text" as const, - text: notice, + text: warning, })), ...result.content, ], diff --git a/apps/ai/src/mcp/__evals__/tool-contract.test.ts b/apps/ai/src/mcp/__evals__/tool-contract.test.ts index b80b317f69..cd0019863b 100644 --- a/apps/ai/src/mcp/__evals__/tool-contract.test.ts +++ b/apps/ai/src/mcp/__evals__/tool-contract.test.ts @@ -38,6 +38,8 @@ beforeAll(() => { rows: [], }, ...rules, + // Any other query (list_services) returns no rows. + { match: () => true, rows: [] }, ]) rt = makeEvalRuntime() }) @@ -81,7 +83,15 @@ describe("a tool declared with define", () => { it("tells the model which keys it ignored, and what it probably meant", async () => { const result = await call("find_errors", { servce: "checkout" }) expect(markdown(result)).toContain( - "Note: `servce` is not a parameter of `find_errors` and was ignored. Did you mean `service`?", + "Warning: `servce` is not a parameter of `find_errors`. It was ignored, and this result is for the call without it. Did you mean `service`?", + ) + }) + + it("warns first, under the title, when a window key it does not take was ignored", async () => { + const result = await call("list_services", { time_range: "today" }) + expect(result.isError).toBeUndefined() + expect(markdown(result)).toMatch( + /^## Services\n\nWarning: `time_range` is not a parameter of `list_services`\. It was ignored, and this result is for the call without it\. Did you mean `start_time`\/`end_time`\?\n\nTime range: /, ) }) diff --git a/apps/ai/src/mcp/lib/decode-issues.test.ts b/apps/ai/src/mcp/lib/decode-issues.test.ts index 52233957c0..0cfd7a7e44 100644 --- a/apps/ai/src/mcp/lib/decode-issues.test.ts +++ b/apps/ai/src/mcp/lib/decode-issues.test.ts @@ -1,7 +1,7 @@ import { describe, expect, it } from "vitest" import { Exit, Schema } from "effect" import { toInputSchema } from "../tools/registry" -import { formatDecodeFailure, normalizeArguments, suggestParameter } from "./decode-issues" +import { argumentWarnings, formatDecodeFailure, normalizeArguments, suggestParameter } from "./decode-issues" import * as P from "./params" const failureFor = (tool: string, schema: Schema.Codec, input: unknown) => { @@ -81,7 +81,34 @@ describe("argument normalization", () => { it("drops unknown keys and reports them", () => { const normalized = normalizeArguments({ servce: "api", limit: 5 }, ["service", "limit"]) expect(normalized.args).toEqual({ limit: 5 }) - expect(normalized.unknown).toEqual([{ key: "servce", suggestion: "service" }]) + expect(normalized.unknown).toEqual([{ key: "servce", suggestions: ["service"] }]) + }) + + it("points a key that names a whole window at both bounds", () => { + const known = ["service", "start_time", "end_time", "limit"] + const unknown = (key: string) => normalizeArguments({ [key]: "today" }, known).unknown + expect(unknown("time_range")).toEqual([ + { key: "time_range", suggestions: ["start_time", "end_time"] }, + ]) + expect(unknown("lookback")).toEqual([{ key: "lookback", suggestions: ["start_time", "end_time"] }]) + // One bound stays one bound. + expect(unknown("start")).toEqual([{ key: "start", suggestions: ["start_time"] }]) + // A closer match that is not a bound wins over the window guess. + const withDuration = [...known, "min_duration_ms", "max_duration_ms"] + expect(normalizeArguments({ min_duration: 5 }, withDuration).unknown).toEqual([ + { key: "min_duration", suggestions: ["min_duration_ms"] }, + ]) + // A tool without a window gets no window suggestion. + expect(normalizeArguments({ time_range: "1h" }, ["service"]).unknown).toEqual([ + { key: "time_range", suggestions: [] }, + ]) + }) + + it("warns that the result is for the call without the ignored key", () => { + const normalized = normalizeArguments({ time_range: "today" }, ["start_time", "end_time"]) + expect(argumentWarnings(normalized, "list_services")).toEqual([ + "`time_range` is not a parameter of `list_services`. It was ignored, and this result is for the call without it. Did you mean `start_time`/`end_time`?", + ]) }) it("fixes the case of an enum value when exactly one published value matches", () => { diff --git a/apps/ai/src/mcp/lib/decode-issues.ts b/apps/ai/src/mcp/lib/decode-issues.ts index 157d6cb93c..62483a9d21 100644 --- a/apps/ai/src/mcp/lib/decode-issues.ts +++ b/apps/ai/src/mcp/lib/decode-issues.ts @@ -15,8 +15,14 @@ export interface NormalizedArguments { readonly args: unknown /** Retired names that were rewritten to their current name. */ readonly renamed: ReadonlyArray - /** Keys the schema does not have, with the parameter each was probably meant to be. */ - readonly unknown: ReadonlyArray<{ readonly key: string; readonly suggestion?: string }> + /** Keys the schema does not have, with the parameters each was probably meant to be. */ + readonly unknown: ReadonlyArray +} + +export interface UnknownArgument { + readonly key: string + /** Empty when no parameter is close; both bounds for a key that names a whole window. */ + readonly suggestions: ReadonlyArray } const isPlainObject = (value: unknown): value is Readonly> => @@ -72,6 +78,43 @@ export const suggestParameter = (key: string, known: ReadonlyArray): str return best?.name } +/** Words that name a whole time window (`time_range`, `lookback`) rather than one of its bounds. */ +const WINDOW_WORDS: ReadonlySet = new Set([ + "range", + "window", + "timerange", + "timeframe", + "timespan", + "period", + "since", + "lookback", + "duration", + "last", + "ago", +]) + +const isWindowKey = (key: string): boolean => + key.toLowerCase() === "time" || words(key).some((word) => WINDOW_WORDS.has(word)) + +const WINDOW_BOUNDS: ReadonlyArray = ["start_time", "end_time"] + +/** + * The parameters an unknown key most plausibly meant: `time_range` is both window bounds. A closer + * match that is not a bound wins, so `min_duration` still points at `min_duration_ms`. + */ +const suggestParameters = (key: string, known: ReadonlyArray): ReadonlyArray => { + const suggestion = suggestParameter(key, known) + const widens = + (suggestion === undefined || WINDOW_BOUNDS.includes(suggestion)) && + isWindowKey(key) && + WINDOW_BOUNDS.every((bound) => known.includes(bound)) + if (widens) return WINDOW_BOUNDS + return suggestion === undefined ? [] : [suggestion] +} + +const didYouMean = (suggestions: ReadonlyArray): string => + suggestions.length === 0 ? "" : ` Did you mean ${suggestions.map((name) => `\`${name}\``).join("/")}?` + /** * Apply aliases and drop keys the schema does not declare. A dropped key is reported rather than * silently ignored: a model that sent `service_name` to a tool without it believed it filtered. @@ -110,7 +153,7 @@ export const normalizeArguments = ( const knownSet = new Set(known) const args: Record = {} const renamed: Array = [] - const unknown: Array<{ key: string; suggestion?: string }> = [] + const unknown: Array = [] for (const [key, value] of Object.entries(input)) { if (knownSet.has(key)) { args[key] = canonicalEnumValue(value, enums.get(key)) @@ -123,20 +166,20 @@ export const normalizeArguments = ( renamed.push([key, target]) continue } - const suggestion = suggestParameter(key, known) - unknown.push(suggestion === undefined ? { key } : { key, suggestion }) + unknown.push({ key, suggestions: suggestParameters(key, known) }) } return { args, renamed, unknown } } -/** Notes for the result text: how the call was read, when that differs from what was sent. */ -export const argumentNotices = (normalized: NormalizedArguments, tool: string): ReadonlyArray => [ - ...normalized.unknown.map(({ key, suggestion }) => - suggestion === undefined - ? `\`${key}\` is not a parameter of \`${tool}\` and was ignored.` - : `\`${key}\` is not a parameter of \`${tool}\` and was ignored. Did you mean \`${suggestion}\`?`, - ), -] +/** + * Warnings for the result text, one per ignored key. The call still ran, so each says the result + * is for the call without that key: a model that sent `time_range` otherwise reads a default window. + */ +export const argumentWarnings = (normalized: NormalizedArguments, tool: string): ReadonlyArray => + normalized.unknown.map( + ({ key, suggestions }) => + `\`${key}\` is not a parameter of \`${tool}\`. It was ignored, and this result is for the call without it.${didYouMean(suggestions)}`, + ) interface ParameterDoc { readonly type: string @@ -216,10 +259,8 @@ export const formatDecodeFailure = ( ? `- Missing required ${describeParameter(name, docs)}` : `- \`${name}\`${nested}: ${message}` }) - const ignored = normalized.unknown.map(({ key, suggestion }) => - suggestion === undefined - ? `- \`${key}\` is not a parameter of this tool.` - : `- \`${key}\` is not a parameter of this tool. Did you mean \`${suggestion}\`?`, + const ignored = normalized.unknown.map( + ({ key, suggestions }) => `- \`${key}\` is not a parameter of this tool.${didYouMean(suggestions)}`, ) const names = [...docs.keys()].map((name) => (required.has(name) ? `${name} (required)` : name)) return [ diff --git a/apps/ai/src/mcp/lib/tool-doc.test.ts b/apps/ai/src/mcp/lib/tool-doc.test.ts index 2e07ac9e53..0b89b57b0f 100644 --- a/apps/ai/src/mcp/lib/tool-doc.test.ts +++ b/apps/ai/src/mcp/lib/tool-doc.test.ts @@ -9,7 +9,8 @@ describe("renderToolDoc", () => { ["Time range", "a to b"], ["Service", undefined], ], - notices: ["`servce` is not a parameter"], + warnings: ["`servce` is not a parameter"], + notices: ["Window narrowed"], blocks: [doc.table(["Error", "Count"], [["Boom | pipe", "3"]])], truncation: { shown: 1, @@ -22,8 +23,9 @@ describe("renderToolDoc", () => { expect(text).toBe( [ "## Errors by Type", + "Warning: `servce` is not a parameter", "Time range: a to b", - "Note: `servce` is not a parameter", + "Note: Window narrowed", "| Error | Count |\n|---|---|\n| Boom \\| pipe | 3 |", "Showing 1 of 9 error types. Next page: `find_errors offset=1`: the next page", 'Next:\n- `error_detail fingerprint="123"`: sample traces', diff --git a/apps/ai/src/mcp/lib/tool-doc.ts b/apps/ai/src/mcp/lib/tool-doc.ts index 4e55cb61ac..953ce075da 100644 --- a/apps/ai/src/mcp/lib/tool-doc.ts +++ b/apps/ai/src/mcp/lib/tool-doc.ts @@ -33,6 +33,8 @@ export type DocBlock = export interface ToolDoc { /** Rendered as the `##` heading. */ readonly title: string + /** Where the call was not run as sent (ignored keys). First under the title, so it is not missed. */ + readonly warnings?: ReadonlyArray /** What the result covers: the resolved window and the filters that applied. One line. */ readonly scope?: ReadonlyArray readonly blocks: ReadonlyArray @@ -45,7 +47,7 @@ export interface ToolDoc { readonly noun: string readonly next?: NextCall } - /** Things the model should know about how its call was read (ignored keys, clamped values). */ + /** Things the model should know about how its call was read (clamped windows, inferred values). */ readonly notices?: ReadonlyArray readonly next?: ReadonlyArray } @@ -122,6 +124,9 @@ const renderScope = (scope: NonNullable): string | undefined = /** Markdown, in a fixed order, with blank lines between sections. */ export const renderToolDoc = (tool: ToolDoc): string => { const sections: Array = [`## ${tool.title}`] + if (tool.warnings !== undefined && tool.warnings.length > 0) { + sections.push(tool.warnings.map((warning) => `Warning: ${warning}`).join("\n")) + } const scope = tool.scope === undefined ? undefined : renderScope(tool.scope) if (scope !== undefined) sections.push(scope) if (tool.notices !== undefined && tool.notices.length > 0) { diff --git a/apps/ai/src/mcp/tools/registry.ts b/apps/ai/src/mcp/tools/registry.ts index f0b8f7fa71..37bea0dac0 100644 --- a/apps/ai/src/mcp/tools/registry.ts +++ b/apps/ai/src/mcp/tools/registry.ts @@ -82,7 +82,7 @@ import { type McpToolResult, } from "./types" import { - argumentNotices, + argumentWarnings, enumValues, formatDecodeFailure, normalizeArguments, @@ -530,7 +530,7 @@ const isValidNextCall = (call: NextCall, surface: McpToolSurface): boolean => { const finish = Effect.fnUntraced(function* ( definition: MapleToolDefinition, run: ToolRun, - notices: ReadonlyArray, + warnings: ReadonlyArray, surface: McpToolSurface, ) { const invalid = nextCallsOf(run.doc).filter((call) => !isValidNextCall(call, surface)) @@ -544,7 +544,7 @@ const finish = Effect.fnUntraced(function* ( } const doc = invalid.length === 0 ? run.doc : filterNextCalls(run.doc, (call) => !invalid.includes(call)) const text = renderToolDocWithinBudget( - notices.length === 0 ? doc : { ...doc, notices: [...notices, ...(doc.notices ?? [])] }, + warnings.length === 0 ? doc : { ...doc, warnings: [...warnings, ...(doc.warnings ?? [])] }, ) const result: McpToolResult = { content: [{ type: "text", text }], @@ -607,7 +607,7 @@ export const executeRegisteredMcpToolUnscoped = Effect.fn("McpToolRegistry.execu return yield* finish( definition, { ...run, doc: windowNotes.decorate(run.doc) }, - argumentNotices(normalized, definition.name), + argumentWarnings(normalized, definition.name), surface, ) }) diff --git a/apps/ai/src/mcp/tools/tool-output.ts b/apps/ai/src/mcp/tools/tool-output.ts index edb21a67d5..c082c606da 100644 --- a/apps/ai/src/mcp/tools/tool-output.ts +++ b/apps/ai/src/mcp/tools/tool-output.ts @@ -212,7 +212,7 @@ const formatNumberOfChars = (chars: number): string => chars >= 1000 ? `${Math.round(chars / 1000)}k` : String(chars) /** - * Render a doc within {@link MAX_TOOL_TEXT_CHARS}. Head (title, scope, notices) and tail (paging, + * Render a doc within {@link MAX_TOOL_TEXT_CHARS}. Head (title, warnings, scope, notices) and tail (paging, * next calls) always survive; body blocks are kept in order and the first that overflows is cut on a * row or line boundary, followed by a notice telling the model how to narrow the call. */