From 5c3086bda07b1bbabf509aabaa145936a08154cd Mon Sep 17 00:00:00 2001 From: Shreyas Sarve Date: Fri, 23 Oct 2026 12:42:19 +0530 Subject: [PATCH] fix(capability): a product clash is a question for the user, not an error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit searchCapability refused an ambiguous request through the same path as a real refusal, so every client rendered the clash in red and agents narrated "the search failed" before asking. The search still does not run, but the outcome now comes back as an ordinary result — status "ask_user", the message, the routing token and the clarify block — and the products in the question, its options and the mismatch text are named by title. Telemetry counts it under ask_user_reason rather than refusal_reason; the tests that detected the gate through isError detect it through status. Stacked on infra-capability. Reapplies 04e267f, bb1a578, a349c75 and 39a71d6, which that branch reverted in c3f6965. Co-Authored-By: Claude Fable 5.1 --- src/tools/capability-registry/register.ts | 48 ++++++++++--- tests/tools/capabilityRegistryE2E.test.ts | 88 ++++++++++++++--------- 2 files changed, 95 insertions(+), 41 deletions(-) diff --git a/src/tools/capability-registry/register.ts b/src/tools/capability-registry/register.ts index 2a8bf5e9..9c7219b9 100644 --- a/src/tools/capability-registry/register.ts +++ b/src/tools/capability-registry/register.ts @@ -290,6 +290,36 @@ export function addCapabilityRegistryTools( return failed(message, extra); }; + /** + * A question for the user, not a failure. + * + * A product clash stops the search just as a refusal does, but it is the EXPECTED path + * for an ambiguous request, and surfacing it as a tool error made every client render + * it in red and every agent narrate "the search failed". It is returned as an ordinary + * result whose `status` says what to do; the search still does not run. Counted in + * telemetry under `ask_user_reason`, next to the refusals it used to sit with. + */ + const askUser = ( + tool: string, + reason: string, + message: string, + extra: Record, + extras?: MCPEventExtras, + ): CallToolResult => { + try { + trackMCP(tool, server.server.getClientVersion()!, undefined, config, { + ask_user_reason: reason, + ...(extras ?? {}), + }); + } catch { + // Telemetry must not decide whether a tool call succeeds. + } + return ok({ status: "ask_user", reason, message, ...extra }); + }; + + /** The product's title for anything put to a person; the key stays the handle. */ + const label = (name: string) => registry.index.products[name]?.title ?? name; + tools.listProducts = server.tool( "listProducts", "List the BrowserStack products this surface can reach: what each one does, the " + @@ -350,9 +380,6 @@ export function addCapabilityRegistryTools( : { verdict: "settled", product: ambiguity.settled }, ) : undefined; - /** The product's title for the question put to a person; the key stays the handle. */ - const label = (name: string) => - registry.index.products[name]?.title ?? name; const routing = ambiguity.products.length > 1 ? { @@ -792,12 +819,15 @@ export function addCapabilityRegistryTools( .filter(Boolean); return { product: name, + ...(registry.index.products[name]?.title + ? { title: registry.index.products[name]?.title } + : {}), summary: registry.index.products[name]?.summary, ...(senses.length ? { shared_terms: senses } : {}), }; }); const named = anchor.products.length > 0; - return refuse( + return askUser( "searchCapability", "product_ambiguous", (blank @@ -809,15 +839,15 @@ export function addCapabilityRegistryTools( (ambiguity.because?.length ? ` (${ambiguity.because.map((t) => `'${t}'`).join(", ")})` : "") + - `, so the request points at ${ambiguity.settled}, not ${product}. ` + + `, so the request points at ${label(ambiguity.settled as string)}, not ${label(product)}. ` + "Search the product the words name, or — if the user meant " + - `${product} — get them to say so and quote them. ` - : `'${subject}' could mean ${ambiguity.products.join(" or ")} — ` + + `${label(product)} — get them to say so and quote them. ` + : `'${subject}' could mean ${ambiguity.products.map(label).join(" or ")} — ` + `${ambiguity.terms.map((t) => `'${t}'`).join(", ")} ` + `${ambiguity.terms.length === 1 ? "belongs" : "belong"} to both. `) + (named ? `The words you quoted (${anchor.terms.map((t) => `'${t}'`).join(", ")}) ` + - `point at ${anchor.products.join(" and ")}, not ${product}. ` + `point at ${anchor.products.map(label).join(" and ")}, not ${label(product)}. ` : user_words ? "Nothing in the words you quoted names a product or a term only one " + "product uses. " @@ -830,7 +860,7 @@ export function addCapabilityRegistryTools( { routing_token: token, clarify: { - question: `Which product do you mean — ${choices.join(" or ")}?`, + question: `Which product do you mean — ${choices.map(label).join(" or ")}?`, shared: ambiguity.terms, options, }, diff --git a/tests/tools/capabilityRegistryE2E.test.ts b/tests/tools/capabilityRegistryE2E.test.ts index 7d8d2e43..a87ff91b 100644 --- a/tests/tools/capabilityRegistryE2E.test.ts +++ b/tests/tools/capabilityRegistryE2E.test.ts @@ -25,9 +25,15 @@ const FIXTURE_DIR = fileURLToPath( */ const SHIPPED_DIR = mkdtempSync(join(tmpdir(), "cap-registry-multi-")); for (const [from, name] of [ - [new URL("../../capability/tm.capability-index.json", import.meta.url), "tm.capability-index.json"], [ - new URL("../fixtures/capability-multi/secondproduct.capability-index.json", import.meta.url), + new URL("../../capability/tm.capability-index.json", import.meta.url), + "tm.capability-index.json", + ], + [ + new URL( + "../fixtures/capability-multi/secondproduct.capability-index.json", + import.meta.url, + ), "secondproduct.capability-index.json", ], ] as const) { @@ -148,9 +154,8 @@ describe("capability registry, end to end through the server factory", () => { it("carries each entity's one-line meaning on the routing call", async () => { const server = await buildServer(); const listed = JSON.parse( - ( - await (server.getTools().listProducts as any).handler({}, {} as any) - ).content[0].text, + (await (server.getTools().listProducts as any).handler({}, {} as any)) + .content[0].text, ); const byName = Object.fromEntries( listed.products[0].entities.map((e: any) => [e.entity, e]), @@ -214,21 +219,26 @@ describe("capability registry, end to end through the server factory", () => { const server = await buildServer(); const search = server.getTools().searchCapability as any; const run = async (args: Record) => { - const r = await search.handler( - { product: "tm", ...args }, - {} as any, - ); - return { blocked: r.isError === true, body: JSON.parse(r.content[0].text) }; + const r = await search.handler({ product: "tm", ...args }, {} as any); + const body = JSON.parse(r.content[0].text); + // The gate is a question for the user, not an error: it stops the search without + // flagging the call as failed. + return { + blocked: body.status === "ask_user" && r.isError !== true, + body, + }; }; const refused = await run({ query: "list all projects" }); expect(refused.blocked).toBe(true); // The refusal has to be actionable: which products, and on which word. - expect(refused.body.error).toContain("secondproduct or tm"); - expect(refused.body.error).toContain("'project'"); - expect(refused.body.error).toMatch(/to the USER/); + // Products are named by title where they have one — tm's is "Test Management"; + // the fixture's second product declares none and reads as its key. + expect(refused.body.message).toContain("secondproduct or Test Management"); + expect(refused.body.message).toContain("'project'"); + expect(refused.body.message).toMatch(/to the USER/); // And it must name the failure mode it exists to stop. - expect(refused.body.error).toMatch(/search each product in turn and merge/); + expect(refused.body.message).toMatch(/search each product in turn and merge/); // A word only one product claims settles the query, so there is nothing to ask — // PROVIDED it settles on the product being searched. @@ -245,10 +255,11 @@ describe("capability registry, end to end through the server factory", () => { // settled, never whether it settled on what was being asked for. const crossed = await run({ query: "check my quota" }); expect(crossed.blocked).toBe(true); - expect(crossed.body.error).toMatch(/points at secondproduct, not tm/); - expect(crossed.body.error).toMatch(/only secondproduct claims \('quota'\)/); + expect(crossed.body.message).toMatch(/points at secondproduct, not Test Management/); + expect(crossed.body.message).toMatch(/only secondproduct claims \('quota'\)/); expect( - (await run({ query: "check my quota", product: "secondproduct" })).blocked, + (await run({ query: "check my quota", product: "secondproduct" })) + .blocked, ).toBe(false); // ONE WAY OUT, AND IT IS NOT A FLAG. `product_choice: 'user_confirmed'` used to open @@ -256,15 +267,21 @@ describe("capability registry, end to end through the server factory", () => { // caller wrote it on every ambiguous prompt with nothing behind it. What opens it now // is the user's own words, checked against the index rather than taken on trust. expect( - (await run({ - query: "list all projects", - user_words: "the test management one", - })).blocked, + ( + await run({ + query: "list all projects", + user_words: "the test management one", + }) + ).blocked, ).toBe(false); // And a flag by that name buys nothing, because there is no longer such an argument. expect( - (await run({ query: "list all projects", product_choice: "user_confirmed" })) - .blocked, + ( + await run({ + query: "list all projects", + product_choice: "user_confirmed", + }) + ).blocked, ).toBe(true); // `entity` IS NOT A WAY OUT, and used to be. It narrows within a product and says @@ -286,7 +303,11 @@ describe("capability registry, end to end through the server factory", () => { process.env.CAPABILITY_REGISTRY_INDEX_DIR = SHIPPED_DIR; const server = await buildServer(); const r: any = await (server.getTools().searchCapability as any).handler( - { query: "list all projects", product: "tm", product_choice: "not_asked" }, + { + query: "list all projects", + product: "tm", + product_choice: "not_asked", + }, {} as any, ); const { clarify } = JSON.parse(r.content[0].text); @@ -302,7 +323,10 @@ describe("capability registry, end to end through the server factory", () => { // word — the two things a user needs to answer without being shown a schema. const tm = clarify.options.find((o: any) => o.product === "tm"); expect(tm.summary).toMatch(/Test Management/); - expect(tm.shared_terms[0]).toMatchObject({ term: "project", entity: "project" }); + expect(tm.shared_terms[0]).toMatchObject({ + term: "project", + entity: "project", + }); expect(tm.shared_terms[0].means).toMatch(/top-level container/); // The second product ships no entity descriptions, so its sense of `project` has no @@ -321,13 +345,13 @@ describe("capability registry, end to end through the server factory", () => { process.env.CAPABILITY_REGISTRY_INDEX_DIR = SHIPPED_DIR; const server = await buildServer(); const search = server.getTools().searchCapability as any; - const blocked = async (query: string) => - ( - await search.handler( - { query, product: "tm", product_choice: "not_asked" }, - {} as any, - ) - ).isError === true; + const blocked = async (query: string) => { + const r = await search.handler( + { query, product: "tm", product_choice: "not_asked" }, + {} as any, + ); + return JSON.parse(r.content[0].text).status === "ask_user"; + }; // QUERY side: the entry is `project`, the user says "projects". Exact containment // missed this — and it is the query that was actually reported.