From ccd142b0163755b9423ed9cf4b7bd356dcb365ff Mon Sep 17 00:00:00 2001 From: Lionel Zoubritzky Date: Thu, 30 Jul 2026 17:14:42 +0200 Subject: [PATCH 1/4] fix: remove structuredContent when isError is set --- docs/mcp-tools.md | 44 +++++-------- scripts/generate-mcp-docs.mjs | 10 +-- src/tools/BaseTool.ts | 1 - test/scripts/generate-mcp-docs.test.ts | 7 +- test/tools/adminexpress.test.ts | 20 +----- test/tools/altitude.test.ts | 20 +----- test/tools/base-tool-error.test.ts | 24 ++----- test/tools/cadastre.test.ts | 20 +----- test/tools/geocode.test.ts | 11 +--- test/tools/helpers/errorAssertions.ts | 39 +++++++++++ test/tools/strict-input.test.ts | 11 +--- test/tools/urbanisme.test.ts | 39 +---------- test/tools/wfs/countFeatures.test.ts | 6 -- test/tools/wfs/describeType.test.ts | 14 +--- test/tools/wfs/getFeatureById.test.ts | 22 ------- test/tools/wfs/getFeatureByIdLayer.test.ts | 17 +---- test/tools/wfs/getFeatures.test.ts | 75 ++++------------------ test/tools/wfs/getFeaturesLayer.test.ts | 18 +----- test/tools/wfs/searchTypes.test.ts | 11 +--- 19 files changed, 97 insertions(+), 312 deletions(-) create mode 100644 test/tools/helpers/errorAssertions.ts diff --git a/docs/mcp-tools.md b/docs/mcp-tools.md index 8ba19602..f8c9cf24 100644 --- a/docs/mcp-tools.md +++ b/docs/mcp-tools.md @@ -5,8 +5,8 @@ Ce document est généré automatiquement à partir des définitions de tools ex ## Contrat d’erreur MCP - En cas d'échec, chaque tool renvoie `isError: true`. -- `content.text` contient le message de détail en français (aligné avec `structuredContent.detail`). -- `structuredContent` contient l'objet canonique exploitable par un client. +- `content.text` contient le message de détail en français. +- Aucun `structuredContent` n'est renvoyé : ce champ est réservé au `outputSchema` du cas de succès. Exemple complet généré automatiquement à partir d'un appel de tool invalide (contrainte de validation) : @@ -21,19 +21,7 @@ Exemple complet généré automatiquement à partir d'un appel de tool invalide "type": "text", "text": "Paramètres invalides : Le paramètre 'text' est requis." } - ], - "structuredContent": { - "type": "urn:geocontext:problem:invalid-tool-params", - "title": "Paramètres d’outil invalides", - "detail": "Paramètres invalides : Le paramètre 'text' est requis.", - "errors": [ - { - "code": "invalid_type", - "detail": "Le paramètre 'text' est requis.", - "name": "text" - } - ] - } + ] } } ``` @@ -180,7 +168,7 @@ Les coordonnées `lon/lat` retournées sont directement réutilisables dans tous | Cas | `content` | `structuredContent` | Relation entre `content` et `structuredContent` | | --- | --- | --- | --- | | Succès | oui | oui | `content[0].text` est `JSON.stringify(structuredContent)`. | -| Erreur | oui | oui | `content[0].text` contient `structuredContent.detail`, pas le JSON d'erreur complet de `structuredContent`. | +| Erreur | oui | non | `content[0].text` porte le message d'erreur ; aucun `structuredContent` n'est ajouté (réservé au `outputSchema` du cas de succès). | ## `altitude` @@ -281,7 +269,7 @@ Renvoie l'altitude (en mètres) et la précision de la mesure (accuracy) d'un po | Cas | `content` | `structuredContent` | Relation entre `content` et `structuredContent` | | --- | --- | --- | --- | | Succès | oui | oui | `content[0].text` est `JSON.stringify(structuredContent)`. | -| Erreur | oui | oui | `content[0].text` contient `structuredContent.detail`, pas le JSON d'erreur complet de `structuredContent`. | +| Erreur | oui | non | `content[0].text` porte le message d'erreur ; aucun `structuredContent` n'est ajouté (réservé au `outputSchema` du cas de succès). | ## `adminexpress` @@ -411,7 +399,7 @@ Pour récupérer exactement l'objet correspondant au `feature_ref`, utiliser `gp | Cas | `content` | `structuredContent` | Relation entre `content` et `structuredContent` | | --- | --- | --- | --- | | Succès | oui | oui | `content[0].text` est `JSON.stringify(structuredContent)`. | -| Erreur | oui | oui | `content[0].text` contient `structuredContent.detail`, pas le JSON d'erreur complet de `structuredContent`. | +| Erreur | oui | non | `content[0].text` porte le message d'erreur ; aucun `structuredContent` n'est ajouté (réservé au `outputSchema` du cas de succès). | ## `cadastre` @@ -552,7 +540,7 @@ Pour récupérer exactement l'objet correspondant au `feature_ref`, utiliser `gp | Cas | `content` | `structuredContent` | Relation entre `content` et `structuredContent` | | --- | --- | --- | --- | | Succès | oui | oui | `content[0].text` est `JSON.stringify(structuredContent)`. | -| Erreur | oui | oui | `content[0].text` contient `structuredContent.detail`, pas le JSON d'erreur complet de `structuredContent`. | +| Erreur | oui | non | `content[0].text` porte le message d'erreur ; aucun `structuredContent` n'est ajouté (réservé au `outputSchema` du cas de succès). | ## `urbanisme` @@ -690,7 +678,7 @@ Modèles d'URL Géoportail de l'Urbanisme : | Cas | `content` | `structuredContent` | Relation entre `content` et `structuredContent` | | --- | --- | --- | --- | | Succès | oui | oui | `content[0].text` est `JSON.stringify(structuredContent)`. | -| Erreur | oui | oui | `content[0].text` contient `structuredContent.detail`, pas le JSON d'erreur complet de `structuredContent`. | +| Erreur | oui | non | `content[0].text` porte le message d'erreur ; aucun `structuredContent` n'est ajouté (réservé au `outputSchema` du cas de succès). | ## `assiette_sup` @@ -824,7 +812,7 @@ Pour récupérer exactement l'objet correspondant au `feature_ref`, utiliser `gp | Cas | `content` | `structuredContent` | Relation entre `content` et `structuredContent` | | --- | --- | --- | --- | | Succès | oui | oui | `content[0].text` est `JSON.stringify(structuredContent)`. | -| Erreur | oui | oui | `content[0].text` contient `structuredContent.detail`, pas le JSON d'erreur complet de `structuredContent`. | +| Erreur | oui | non | `content[0].text` porte le message d'erreur ; aucun `structuredContent` n'est ajouté (réservé au `outputSchema` du cas de succès). | ## `gpf_search_types` @@ -954,7 +942,7 @@ Le paramètre `max_results` permet d'élargir le nombre de candidats retournés | Cas | `content` | `structuredContent` | Relation entre `content` et `structuredContent` | | --- | --- | --- | --- | | Succès | oui | oui | `content[0].text` est `JSON.stringify(structuredContent)`. | -| Erreur | oui | oui | `content[0].text` contient `structuredContent.detail`, pas le JSON d'erreur complet de `structuredContent`. | +| Erreur | oui | non | `content[0].text` porte le message d'erreur ; aucun `structuredContent` n'est ajouté (réservé au `outputSchema` du cas de succès). | ## `gpf_describe_type` @@ -1080,7 +1068,7 @@ Utiliser ce tool après `gpf_search_types` pour inspecter les propriétés dispo | Cas | `content` | `structuredContent` | Relation entre `content` et `structuredContent` | | --- | --- | --- | --- | | Succès | oui | oui | `content[0].text` est `JSON.stringify(structuredContent)`. | -| Erreur | oui | oui | `content[0].text` contient `structuredContent.detail`, pas le JSON d'erreur complet de `structuredContent`. | +| Erreur | oui | non | `content[0].text` porte le message d'erreur ; aucun `structuredContent` n'est ajouté (réservé au `outputSchema` du cas de succès). | ## `gpf_get_features` @@ -1414,7 +1402,7 @@ Aucun `outputSchema` unique n'est exposé. La sortie est gérée par la sériali | Cas | `content` | `structuredContent` | Relation entre `content` et `structuredContent` | | --- | --- | --- | --- | | Succès | oui | non | `content[0].text` est la FeatureCollection stringifiée (propriétés attributaires uniquement) ; aucun `structuredContent` n'est ajouté. | -| Erreur | oui | oui | `content[0].text` contient `structuredContent.detail`, pas le JSON d'erreur complet de `structuredContent`. | +| Erreur | oui | non | `content[0].text` porte le message d'erreur ; aucun `structuredContent` n'est ajouté (réservé au `outputSchema` du cas de succès). | ## `gpf_get_features_layer` @@ -1746,7 +1734,7 @@ Mêmes filtres que `gpf_get_features` : `select` pour choisir les propriétés, | Cas | `content` | `structuredContent` | Relation entre `content` et `structuredContent` | | --- | --- | --- | --- | | Succès | oui | oui | `content[0].text` est `JSON.stringify(structuredContent)`. | -| Erreur | oui | oui | `content[0].text` contient `structuredContent.detail`, pas le JSON d'erreur complet de `structuredContent`. | +| Erreur | oui | non | `content[0].text` porte le message d'erreur ; aucun `structuredContent` n'est ajouté (réservé au `outputSchema` du cas de succès). | ## `gpf_count_features` @@ -2036,7 +2024,7 @@ Les noms de propriétés utilisés dans `where` **ne peuvent pas être devinés* | Cas | `content` | `structuredContent` | Relation entre `content` et `structuredContent` | | --- | --- | --- | --- | | Succès | oui | oui | `content[0].text` est `JSON.stringify(structuredContent)`. | -| Erreur | oui | oui | `content[0].text` contient `structuredContent.detail`, pas le JSON d'erreur complet de `structuredContent`. | +| Erreur | oui | non | `content[0].text` porte le message d'erreur ; aucun `structuredContent` n'est ajouté (réservé au `outputSchema` du cas de succès). | ## `gpf_get_feature_by_id` @@ -2125,7 +2113,7 @@ Aucun `outputSchema` unique n'est exposé. La sortie est gérée par la sériali | Cas | `content` | `structuredContent` | Relation entre `content` et `structuredContent` | | --- | --- | --- | --- | | Succès | oui | oui | `content[0].text` est la FeatureCollection stringifiée, également exposée dans `structuredContent`. | -| Erreur | oui | oui | `content[0].text` contient `structuredContent.detail`, pas le JSON d'erreur complet de `structuredContent`. | +| Erreur | oui | non | `content[0].text` porte le message d'erreur ; aucun `structuredContent` n'est ajouté (réservé au `outputSchema` du cas de succès). | ## `gpf_get_feature_by_id_layer` @@ -2223,4 +2211,4 @@ Cet outil ne peut renvoyer qu'un unique objet (0 ou plusieurs résultats provoqu | Cas | `content` | `structuredContent` | Relation entre `content` et `structuredContent` | | --- | --- | --- | --- | | Succès | oui | oui | `content[0].text` est `JSON.stringify(structuredContent)`. | -| Erreur | oui | oui | `content[0].text` contient `structuredContent.detail`, pas le JSON d'erreur complet de `structuredContent`. | +| Erreur | oui | non | `content[0].text` porte le message d'erreur ; aucun `structuredContent` n'est ajouté (réservé au `outputSchema` du cas de succès). | diff --git a/scripts/generate-mcp-docs.mjs b/scripts/generate-mcp-docs.mjs index fbffd4e0..4b7a4706 100644 --- a/scripts/generate-mcp-docs.mjs +++ b/scripts/generate-mcp-docs.mjs @@ -191,8 +191,8 @@ export function renderResponseContractSection(definition) { const errorRow = { caseName: "Erreur", content: "oui", - structuredContent: "oui", - relation: "`content[0].text` contient `structuredContent.detail`, pas le JSON d'erreur complet de `structuredContent`.", + structuredContent: "non", + relation: "`content[0].text` porte le message d'erreur ; aucun `structuredContent` n'est ajouté (réservé au `outputSchema` du cas de succès).", }; if (definition.name === "gpf_get_features") { @@ -489,6 +489,7 @@ export function buildValidationErrorExampleForTool(tool, normalizeToolError) { } const payload = normalizeToolError(result.error); + // Mirrors `BaseTool.createErrorResponse`, which omits `structuredContent`. const response = normalizeErrorResponse({ isError: true, content: [ @@ -497,7 +498,6 @@ export function buildValidationErrorExampleForTool(tool, normalizeToolError) { text: String(payload.detail ?? "Erreur de validation."), }, ], - structuredContent: payload, }); if (!response) { @@ -542,8 +542,8 @@ async function buildErrorContractSection(tools) { "## Contrat d’erreur MCP", "", "- En cas d'échec, chaque tool renvoie `isError: true`.", - "- `content.text` contient le message de détail en français (aligné avec `structuredContent.detail`).", - "- `structuredContent` contient l'objet canonique exploitable par un client.", + "- `content.text` contient le message de détail en français.", + "- Aucun `structuredContent` n'est renvoyé : ce champ est réservé au `outputSchema` du cas de succès.", "", "Exemple complet généré automatiquement à partir d'un appel de tool invalide (contrainte de validation) :", "", diff --git a/src/tools/BaseTool.ts b/src/tools/BaseTool.ts index d1338322..7480a88d 100644 --- a/src/tools/BaseTool.ts +++ b/src/tools/BaseTool.ts @@ -47,7 +47,6 @@ export default abstract class BaseTool = any> text: payload.detail, }, ], - structuredContent: payload as Record, isError: true, }; } diff --git a/test/scripts/generate-mcp-docs.test.ts b/test/scripts/generate-mcp-docs.test.ts index 42afff92..bcb52e79 100644 --- a/test/scripts/generate-mcp-docs.test.ts +++ b/test/scripts/generate-mcp-docs.test.ts @@ -129,7 +129,7 @@ describe("generate-mcp-docs helpers", () => { expect(markdown).toContain("### Réponse MCP"); expect(markdown).toContain("| Succès | oui | oui | `content[0].text` est `JSON.stringify(structuredContent)`. |"); - expect(markdown).toContain("| Erreur | oui | oui | `content[0].text` contient `structuredContent.detail`, pas le JSON d'erreur complet de `structuredContent`. |"); + expect(markdown).toContain("| Erreur | oui | non | `content[0].text` porte le message d'erreur ; aucun `structuredContent` n'est ajouté (réservé au `outputSchema` du cas de succès). |"); }); it("should document the get features single-mode response contract", async () => { @@ -240,11 +240,8 @@ describe("generate-mcp-docs helpers", () => { text: expect.stringContaining("Paramètres invalides"), }, ], - structuredContent: { - type: "urn:geocontext:problem:invalid-tool-params", - title: "Paramètres d’outil invalides", - }, }, }); + expect(example?.response.structuredContent).toBeUndefined(); }); }); diff --git a/test/tools/adminexpress.test.ts b/test/tools/adminexpress.test.ts index 963b0660..f8649a47 100644 --- a/test/tools/adminexpress.test.ts +++ b/test/tools/adminexpress.test.ts @@ -2,6 +2,7 @@ import { describe, it, expect } from "vitest"; import AdminexpressTool from "../../src/tools/AdminexpressTool"; import { validateStructuredContentAgainstOutputSchema } from "./helpers/outputSchema"; +import { expectInvalidLon } from "./helpers/errorAssertions"; import { mairieLoray } from "../samples"; const adminexpressResults = [ @@ -142,23 +143,6 @@ describe("Test AdminexpressTool",() => { }, }); - expect(response.isError).toBe(true); - expect(response.content[0]).toMatchObject({ - type: "text", - }); - const textContent = response.content[0]; - if (textContent.type !== "text") { - throw new Error("expected text content"); - } - expect(textContent.text).toContain("Paramètres invalides"); - expect(response.structuredContent).toMatchObject({ - type: "urn:geocontext:problem:invalid-tool-params", - errors: expect.arrayContaining([ - expect.objectContaining({ - name: "lon", - code: "too_big", - }), - ]), - }); + expectInvalidLon(response); }); }); diff --git a/test/tools/altitude.test.ts b/test/tools/altitude.test.ts index 11f74ac8..ee3f003a 100644 --- a/test/tools/altitude.test.ts +++ b/test/tools/altitude.test.ts @@ -2,6 +2,7 @@ import { describe, it, expect } from "vitest"; import AltitudeTool from "../../src/tools/AltitudeTool"; import { validateStructuredContentAgainstOutputSchema } from "./helpers/outputSchema"; +import { expectInvalidLon } from "./helpers/errorAssertions"; import { paris } from "../samples"; describe("Test AltitudeTool",() => { @@ -82,23 +83,6 @@ describe("Test AltitudeTool",() => { }, }); - expect(response.isError).toBe(true); - expect(response.content[0]).toMatchObject({ - type: "text", - }); - const textContent = response.content[0]; - if (textContent.type !== "text") { - throw new Error("expected text content"); - } - expect(textContent.text).toContain("Paramètres invalides"); - expect(response.structuredContent).toMatchObject({ - type: "urn:geocontext:problem:invalid-tool-params", - errors: expect.arrayContaining([ - expect.objectContaining({ - name: "lon", - code: "too_big", - }), - ]), - }); + expectInvalidLon(response); }); }); diff --git a/test/tools/base-tool-error.test.ts b/test/tools/base-tool-error.test.ts index 5daf7eff..a4f612e0 100644 --- a/test/tools/base-tool-error.test.ts +++ b/test/tools/base-tool-error.test.ts @@ -50,6 +50,8 @@ describe("Test BaseTool error response", () => { expect.stringContaining("[tool] failed dummy_error_tool: Paramètres invalides"), expect.objectContaining({ tool: "dummy_error_tool", + problem_type: "urn:geocontext:problem:invalid-tool-params", + problem_title: "Paramètres d’outil invalides", input: { lon: 600, }, @@ -61,15 +63,8 @@ describe("Test BaseTool error response", () => { type: "text", text: expect.stringContaining("Paramètres invalides"), }); - expect(response.structuredContent).toMatchObject({ - type: "urn:geocontext:problem:invalid-tool-params", - errors: expect.arrayContaining([ - expect.objectContaining({ - name: "lon", - code: "too_big", - }), - ]), - }); + // Reserved for the success-path `outputSchema`; never set on errors. + expect(response.structuredContent).toBeUndefined(); }); it("should return normalized runtime errors", async () => { @@ -99,16 +94,7 @@ describe("Test BaseTool error response", () => { type: "text", text: "runtime failure for lon=2.3", }); - expect(response.structuredContent).toMatchObject({ - type: "urn:geocontext:problem:execution-error", - detail: "runtime failure for lon=2.3", - errors: [ - { - code: "execution_error", - detail: "runtime failure for lon=2.3", - }, - ], - }); + expect(response.structuredContent).toBeUndefined(); }); it("should keep the matching input in concurrent runtime error logs", async () => { diff --git a/test/tools/cadastre.test.ts b/test/tools/cadastre.test.ts index b1539bcb..d244a22a 100644 --- a/test/tools/cadastre.test.ts +++ b/test/tools/cadastre.test.ts @@ -2,6 +2,7 @@ import { describe, it, expect } from "vitest"; import CadastreTool from "../../src/tools/CadastreTool"; import { validateStructuredContentAgainstOutputSchema } from "./helpers/outputSchema"; +import { expectInvalidLon } from "./helpers/errorAssertions"; import { mairieLoray } from "../samples"; describe("Test CadastreTool",() => { @@ -127,23 +128,6 @@ describe("Test CadastreTool",() => { }, }); - expect(response.isError).toBe(true); - expect(response.content[0]).toMatchObject({ - type: "text", - }); - const textContent = response.content[0]; - if (textContent.type !== "text") { - throw new Error("expected text content"); - } - expect(textContent.text).toContain("Paramètres invalides"); - expect(response.structuredContent).toMatchObject({ - type: "urn:geocontext:problem:invalid-tool-params", - errors: expect.arrayContaining([ - expect.objectContaining({ - name: "lon", - code: "too_big", - }), - ]), - }); + expectInvalidLon(response); }); }); diff --git a/test/tools/geocode.test.ts b/test/tools/geocode.test.ts index 5226cacc..510095c7 100644 --- a/test/tools/geocode.test.ts +++ b/test/tools/geocode.test.ts @@ -99,15 +99,6 @@ describe("Test GeocodeTool",() => { throw new Error("expected text content"); } expect(textContent.text).toContain("Paramètres invalides"); - expect(response.structuredContent).toMatchObject({ - type: "urn:geocontext:problem:invalid-tool-params", - errors: expect.arrayContaining([ - expect.objectContaining({ - name: "text", - code: "too_small", - detail: "le texte ne doit pas être vide", - }), - ]), - }); + expect(textContent.text).toContain("le texte ne doit pas être vide"); }); }); diff --git a/test/tools/helpers/errorAssertions.ts b/test/tools/helpers/errorAssertions.ts new file mode 100644 index 00000000..5aa2382f --- /dev/null +++ b/test/tools/helpers/errorAssertions.ts @@ -0,0 +1,39 @@ +import { expect } from "vitest"; + +/** + * Shared assertions over tool error responses, keeping the expected shape and + * FR wording in one place rather than in every tool test. + */ + +type ToolResponse = { + isError?: boolean; + structuredContent?: unknown; + content: Array<{ type: string; text?: string }>; +}; + +/** + * Extracts the text of an error response, asserting its shape along the way. + * + * @param response Tool response expected to be an error. + * @returns The error message text. + */ +export function expectErrorText(response: ToolResponse) { + expect(response.isError).toBe(true); + // Reserved for the success-path `outputSchema`; never set on errors. + expect(response.structuredContent).toBeUndefined(); + + const textContent = response.content[0]; + if (textContent?.type !== "text" || typeof textContent.text !== "string") { + throw new Error("expected text content"); + } + return textContent.text; +} + +/** + * Asserts that a response rejected an out-of-range `lon`. + * + * @param response Tool response expected to be a validation error. + */ +export function expectInvalidLon(response: ToolResponse) { + expect(expectErrorText(response)).toContain("Paramètres invalides"); +} diff --git a/test/tools/strict-input.test.ts b/test/tools/strict-input.test.ts index 20bc8e3a..f9398bff 100644 --- a/test/tools/strict-input.test.ts +++ b/test/tools/strict-input.test.ts @@ -94,15 +94,6 @@ describe("Strict tool input schemas", () => { throw new Error("expected text content"); } expect(textContent.text).toContain("Paramètres invalides"); - expect(response.structuredContent).toMatchObject({ - type: "urn:geocontext:problem:invalid-tool-params", - errors: expect.arrayContaining([ - expect.objectContaining({ - code: "unknown_parameter", - name: "unexpected", - detail: expect.stringContaining("unexpected"), - }), - ]), - }); + expect(textContent.text).toContain("Le paramètre 'unexpected' n'est pas reconnu."); }); }); diff --git a/test/tools/urbanisme.test.ts b/test/tools/urbanisme.test.ts index 8b49968d..1a70d996 100644 --- a/test/tools/urbanisme.test.ts +++ b/test/tools/urbanisme.test.ts @@ -3,6 +3,7 @@ import { describe, it, expect } from "vitest"; import AssietteSupTool from "../../src/tools/AssietteSupTool"; import UrbanismeTool from "../../src/tools/UrbanismeTool"; import { validateStructuredContentAgainstOutputSchema } from "./helpers/outputSchema"; +import { expectInvalidLon } from "./helpers/errorAssertions"; import { chamonix, mairieLoray } from "../samples"; describe("Test UrbanismeTool",() => { @@ -124,24 +125,7 @@ describe("Test UrbanismeTool",() => { }, }); - expect(response.isError).toBe(true); - expect(response.content[0]).toMatchObject({ - type: "text", - }); - const textContent = response.content[0]; - if (textContent.type !== "text") { - throw new Error("expected text content"); - } - expect(textContent.text).toContain("Paramètres invalides"); - expect(response.structuredContent).toMatchObject({ - type: "urn:geocontext:problem:invalid-tool-params", - errors: expect.arrayContaining([ - expect.objectContaining({ - name: "lon", - code: "too_big", - }), - ]), - }); + expectInvalidLon(response); }); }); @@ -264,23 +248,6 @@ describe("Test AssietteSupTool",() => { }, }); - expect(response.isError).toBe(true); - expect(response.content[0]).toMatchObject({ - type: "text", - }); - const textContent = response.content[0]; - if (textContent.type !== "text") { - throw new Error("expected text content"); - } - expect(textContent.text).toContain("Paramètres invalides"); - expect(response.structuredContent).toMatchObject({ - type: "urn:geocontext:problem:invalid-tool-params", - errors: expect.arrayContaining([ - expect.objectContaining({ - name: "lon", - code: "too_big", - }), - ]), - }); + expectInvalidLon(response); }); }); diff --git a/test/tools/wfs/countFeatures.test.ts b/test/tools/wfs/countFeatures.test.ts index ef5b5895..54e1dc6e 100644 --- a/test/tools/wfs/countFeatures.test.ts +++ b/test/tools/wfs/countFeatures.test.ts @@ -221,9 +221,6 @@ describe("Test GpfCountFeaturesTool", () => { } expect(textContent.text).toContain("catalogue embarqué est rejeté"); expect(textContent.text).toContain("géométrique 'geometrie'"); - expect(response.structuredContent).toMatchObject({ - type: "urn:geocontext:problem:execution-error", - }); }); it.each([ @@ -249,8 +246,5 @@ describe("Test GpfCountFeaturesTool", () => { throw new Error("expected text content"); } expect(textContent.text).toContain(errorMessage); - expect(response.structuredContent).toMatchObject({ - type: "urn:geocontext:problem:execution-error", - }); }); }); \ No newline at end of file diff --git a/test/tools/wfs/describeType.test.ts b/test/tools/wfs/describeType.test.ts index 7ed0db94..9336ba3d 100644 --- a/test/tools/wfs/describeType.test.ts +++ b/test/tools/wfs/describeType.test.ts @@ -115,16 +115,7 @@ describe("Test GpfDescribeTypeTool",() => { throw new Error("expected text content"); } expect(textContent.text).toContain("Paramètres invalides"); - expect(response.structuredContent).toMatchObject({ - type: "urn:geocontext:problem:invalid-tool-params", - errors: expect.arrayContaining([ - expect.objectContaining({ - name: "typename", - code: "too_small", - detail: "le nom du type ne doit pas être vide", - }), - ]), - }); + expect(textContent.text).toContain("le nom du type ne doit pas être vide"); }); it("should return isError=true when execute fails", async () => { @@ -148,8 +139,5 @@ describe("Test GpfDescribeTypeTool",() => { } expect(textContent.text).toContain("Le type 'BDTOPO_V3:not_found' est introuvable"); expect(textContent.text).toContain("gpf_search_types"); - expect(response.structuredContent).toMatchObject({ - type: "urn:geocontext:problem:execution-error", - }); }); }); diff --git a/test/tools/wfs/getFeatureById.test.ts b/test/tools/wfs/getFeatureById.test.ts index 01d09f33..45e3e777 100644 --- a/test/tools/wfs/getFeatureById.test.ts +++ b/test/tools/wfs/getFeatureById.test.ts @@ -327,12 +327,6 @@ describe("Test GpfGetFeatureByIdTool", () => { } expect(textContent.text).toContain("est introuvable"); expect(textContent.text).toContain("commune.404"); - expect(response.structuredContent).toMatchObject({ - type: "urn:geocontext:problem:feature-not-found", - errors: expect.arrayContaining([ - expect.objectContaining({ code: "feature_not_found" }), - ]), - }); }); it("should fail clearly when multiple features are returned", async () => { @@ -363,12 +357,6 @@ describe("Test GpfGetFeatureByIdTool", () => { throw new Error("expected text content"); } expect(textContent.text).toContain("devrait être unique"); - expect(response.structuredContent).toMatchObject({ - type: "urn:geocontext:problem:feature-cardinality", - errors: expect.arrayContaining([ - expect.objectContaining({ code: "feature_cardinality" }), - ]), - }); }); it("should fail clearly when the returned feature id mismatches", async () => { @@ -398,12 +386,6 @@ describe("Test GpfGetFeatureByIdTool", () => { throw new Error("expected text content"); } expect(textContent.text).toContain("au lieu de"); - expect(response.structuredContent).toMatchObject({ - type: "urn:geocontext:problem:feature-cardinality", - errors: expect.arrayContaining([ - expect.objectContaining({ code: "feature_cardinality" }), - ]), - }); }); it("should fail clearly when execution returns an unexpected success payload", async () => { @@ -426,10 +408,6 @@ describe("Test GpfGetFeatureByIdTool", () => { } expect(textContent.text).toContain("Réponse interne inattendue"); expect(textContent.text).toContain("FeatureCollection"); - expect(response.structuredContent).toMatchObject({ - type: "urn:geocontext:problem:execution-error", - detail: expect.stringContaining("gpf_get_feature_by_id"), - }); }); it("should work on a geometry-less table when select and spatial_extras are empty", async () => { diff --git a/test/tools/wfs/getFeatureByIdLayer.test.ts b/test/tools/wfs/getFeatureByIdLayer.test.ts index 5c041713..27209a66 100644 --- a/test/tools/wfs/getFeatureByIdLayer.test.ts +++ b/test/tools/wfs/getFeatureByIdLayer.test.ts @@ -7,6 +7,7 @@ import { validateStructuredContentAgainstOutputSchema } from "../helpers/outputS import type { Env } from "../../../src/config/env.js"; import { decodeToken } from "../../../src/proxy/token.js"; import { PROXY_TOKEN_KIND } from "../../../src/wfs/schema.js"; +import { expectErrorText } from "../helpers/errorAssertions"; // 32-byte key as 64 hex chars, decoded to a Buffer the way env.ts would. const SECRET_HEX = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"; @@ -243,13 +244,7 @@ describe("Test GpfGetFeatureByIdLayerTool", () => { }, }); - expect(response.isError).toBe(true); - expect(response.structuredContent).toMatchObject({ - type: "urn:geocontext:problem:invalid-tool-params", - errors: expect.arrayContaining([ - expect.objectContaining({ name: "where", code: "unknown_parameter" }), - ]), - }); + expect(expectErrorText(response)).toContain("Le paramètre 'where' n'est pas reconnu."); }); it("rejects an unknown selected property BEFORE minting the URL", async () => { @@ -306,12 +301,6 @@ describe("Test GpfGetFeatureByIdLayerTool", () => { }, }); - expect(response.isError).toBe(true); - expect(response.structuredContent).toMatchObject({ - type: "urn:geocontext:problem:invalid-tool-params", - errors: expect.arrayContaining([ - expect.objectContaining({ name: "feature_id" }), - ]), - }); + expect(expectErrorText(response)).toContain("Le paramètre 'feature_id' est requis."); }); }); diff --git a/test/tools/wfs/getFeatures.test.ts b/test/tools/wfs/getFeatures.test.ts index be175fbf..8e82767d 100644 --- a/test/tools/wfs/getFeatures.test.ts +++ b/test/tools/wfs/getFeatures.test.ts @@ -3,6 +3,7 @@ import { vi, describe, it, expect, afterEach } from "vitest"; import type { OgcCollectionSchema } from "@ignfab/gpf-schema-store"; import type { GpfFeatureType } from "../../../src/wfs/catalog.js"; import { ServiceResponseError } from "../../../src/helpers/http.js"; +import { expectErrorText } from "../helpers/errorAssertions"; const mockGetFeatureType = vi.fn<(typename: string) => Promise>(); const mockFetchJSONPost = vi.fn<( @@ -327,16 +328,7 @@ describe("Test GpfGetFeaturesTool", () => { throw new Error("expected text content"); } expect(textContent.text).toContain("Paramètres invalides"); - expect(response.structuredContent).toMatchObject({ - type: "urn:geocontext:problem:invalid-tool-params", - errors: expect.arrayContaining([ - expect.objectContaining({ - name: "typename", - code: "too_small", - detail: "le nom du type ne doit pas être vide", - }), - ]), - }); + expect(textContent.text).toContain("le nom du type ne doit pas être vide"); expect(tool.toolDefinition.outputSchema).toBeUndefined(); }); @@ -373,16 +365,7 @@ describe("Test GpfGetFeaturesTool", () => { } expect(textContent.text).toContain("Paramètres invalides"); expect(textContent.text).toContain("Un seul filtre spatial est autorisé"); - expect(response.structuredContent).toMatchObject({ - type: "urn:geocontext:problem:invalid-tool-params", - errors: expect.arrayContaining([ - expect.objectContaining({ - code: "custom", - name: "spatial_filters", - detail: expect.stringContaining("bbox_filter, intersects_point_filter"), - }), - ]), - }); + expect(textContent.text).toContain("bbox_filter, intersects_point_filter"); expect(mockGetFeatureType).not.toHaveBeenCalled(); expect(mockFetchJSONPost).not.toHaveBeenCalled(); }); @@ -403,17 +386,9 @@ describe("Test GpfGetFeaturesTool", () => { it.each(FILTER_DEPENDENT_EXTRAS)("should reject %s without a spatial filter as invalid tool parameters", async (extra) => { const response = await callWith({ spatial_extras: [extra] }); - expect(response.isError).toBe(true); - expect(response.structuredContent).toMatchObject({ - type: "urn:geocontext:problem:invalid-tool-params", - errors: [ - expect.objectContaining({ - code: "custom", - name: "spatial_extras", - detail: expect.stringContaining(`\`${extra}\` exige un filtre spatial`), - }), - ], - }); + const errorText = expectErrorText(response); + expect(errorText).toContain("spatial_extras"); + expect(errorText).toContain(`\`${extra}\` exige un filtre spatial`); expect(mockGetFeatureType).not.toHaveBeenCalled(); expect(mockFetchJSONPost).not.toHaveBeenCalled(); }); @@ -424,17 +399,10 @@ describe("Test GpfGetFeaturesTool", () => { spatial_extras: [extra], }); - expect(response.isError).toBe(true); - expect(response.structuredContent).toMatchObject({ - type: "urn:geocontext:problem:invalid-tool-params", - errors: [ - expect.objectContaining({ - code: "custom", - name: "spatial_extras", - detail: expect.stringContaining("intersects_point_filter"), - }), - ], - }); + const errorText = expectErrorText(response); + expect(errorText).toContain("spatial_extras"); + expect(errorText).toContain(`\`${extra}\``); + expect(errorText).toContain("intersects_point_filter"); expect(mockGetFeatureType).not.toHaveBeenCalled(); expect(mockFetchJSONPost).not.toHaveBeenCalled(); }); @@ -473,16 +441,7 @@ describe("Test GpfGetFeaturesTool", () => { throw new Error("expected text content"); } expect(textContent.text).toContain("Paramètres invalides"); - expect(response.structuredContent).toMatchObject({ - type: "urn:geocontext:problem:invalid-tool-params", - errors: expect.arrayContaining([ - expect.objectContaining({ - code: "unknown_parameter", - name: "cql_filter", - detail: expect.stringContaining("cql_filter"), - }), - ]), - }); + expect(textContent.text).toContain("Le paramètre 'cql_filter' n'est pas reconnu."); }); it("should build a POST request with query params and encoded body", async () => { @@ -553,9 +512,6 @@ describe("Test GpfGetFeaturesTool", () => { } expect(textContent.text).toContain("catalogue embarqué est rejeté"); expect(textContent.text).toContain("géométrique 'geometrie'"); - expect(response.structuredContent).toMatchObject({ - type: "urn:geocontext:problem:execution-error", - }); }); it("should return feature_ref for non point layers with geometry set to null", async () => { @@ -800,12 +756,6 @@ describe("Test GpfGetFeaturesTool", () => { } expect(textContent.text).toContain("est introuvable"); expect(textContent.text).toContain("localisant.404"); - expect(response.structuredContent).toMatchObject({ - type: "urn:geocontext:problem:feature-not-found", - errors: expect.arrayContaining([ - expect.objectContaining({ code: "feature_not_found" }), - ]), - }); }); describe("extras that depend on the intersects_feature reference", () => { @@ -909,9 +859,6 @@ describe("Test GpfGetFeaturesTool", () => { } expect(textContent.text).toContain("gpf_get_feature_by_id"); expect(textContent.text).toContain("intersects_feature"); - expect(response.structuredContent).toMatchObject({ - type: "urn:geocontext:problem:execution-error", - }); expect(requests).toHaveLength(0); }); diff --git a/test/tools/wfs/getFeaturesLayer.test.ts b/test/tools/wfs/getFeaturesLayer.test.ts index 5afe6cac..90dd7085 100644 --- a/test/tools/wfs/getFeaturesLayer.test.ts +++ b/test/tools/wfs/getFeaturesLayer.test.ts @@ -7,6 +7,7 @@ import { validateStructuredContentAgainstOutputSchema } from "../helpers/outputS import type { Env } from "../../../src/config/env.js"; import { decodeToken } from "../../../src/proxy/token.js"; import { PROXY_TOKEN_KIND } from "../../../src/wfs/schema.js"; +import { expectErrorText } from "../helpers/errorAssertions"; // 32-byte key as 64 hex chars, decoded to a Buffer the way env.ts would. const SECRET_HEX = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"; @@ -226,13 +227,7 @@ describe("Test GpfGetFeaturesLayerTool", () => { }, }); - expect(response.isError).toBe(true); - expect(response.structuredContent).toMatchObject({ - type: "urn:geocontext:problem:invalid-tool-params", - errors: expect.arrayContaining([ - expect.objectContaining({ name: "spatial_extras", code: "unknown_parameter" }), - ]), - }); + expect(expectErrorText(response)).toContain("Le paramètre 'spatial_extras' n'est pas reconnu."); // Zod validation fails before the catalog pre-flight. expect(mockGetFeatureType).not.toHaveBeenCalled(); }); @@ -252,8 +247,7 @@ describe("Test GpfGetFeaturesLayerTool", () => { }, }); - expect(response.isError).toBe(true); - expect(response.structuredContent?.type).toBe("urn:geocontext:problem:invalid-tool-params") + expect(expectErrorText(response)).toContain("Un seul filtre spatial est autorisé"); }); it("rejects a typename with NO geometry column BEFORE minting the URL (catalog pre-flight)", async () => { @@ -433,11 +427,5 @@ describe("Test GpfGetFeaturesLayerTool", () => { } // FR message, not the EN codec message. expect(textContent.text).toContain("trop volumineuse"); - expect(response.structuredContent).toMatchObject({ - type: "urn:geocontext:problem:proxy-url-error", - errors: expect.arrayContaining([ - expect.objectContaining({ code: "proxy_url_too_large" }), - ]), - }); }); }); diff --git a/test/tools/wfs/searchTypes.test.ts b/test/tools/wfs/searchTypes.test.ts index 44e80c7f..9442f19a 100644 --- a/test/tools/wfs/searchTypes.test.ts +++ b/test/tools/wfs/searchTypes.test.ts @@ -153,15 +153,6 @@ describe("Test GpfSearchTypesTool",() => { throw new Error("expected text content"); } expect(textContent.text).toContain("Paramètres invalides"); - expect(response.structuredContent).toMatchObject({ - type: "urn:geocontext:problem:invalid-tool-params", - errors: expect.arrayContaining([ - expect.objectContaining({ - name: "query", - code: "too_small", - detail: "la requête de recherche ne doit pas être vide", - }), - ]), - }); + expect(textContent.text).toContain("la requête de recherche ne doit pas être vide"); }); }); From 174b0336e860f0cd42adb2683dacf8102107cf54 Mon Sep 17 00:00:00 2001 From: Lionel Zoubritzky Date: Fri, 21 Aug 2026 18:23:45 +0200 Subject: [PATCH 2/4] fix: name the offending param in every error message --- src/errors/toolError.ts | 43 +++++------ src/errors/zodErrorMapFr.ts | 33 ++++++-- test/errors/toolError.test.ts | 105 +++++++++++++++++++++++++- test/tools/helpers/errorAssertions.ts | 5 +- 4 files changed, 154 insertions(+), 32 deletions(-) diff --git a/src/errors/toolError.ts b/src/errors/toolError.ts index c47dcfae..a6bf22cf 100644 --- a/src/errors/toolError.ts +++ b/src/errors/toolError.ts @@ -2,8 +2,12 @@ * Centralized normalization for MCP tool errors. * * This helper converts heterogeneous runtime errors (Zod validation errors, - * upstream service errors, and generic exceptions) into one stable - * `structuredContent` contract consumed by tools through `BaseTool`. + * upstream service errors, and generic exceptions) into one stable problem + * payload. + * + * Error responses carry no `structuredContent` (reserved for the success-path + * `outputSchema`), so `detail` is the only channel the caller sees: it must + * name the offending parameter and stay self-sufficient. */ import { ZodError } from "zod"; @@ -15,7 +19,7 @@ import { ProxyTokenTooLargeError, } from "../proxy/token.js"; import { FeatureNotFoundError, FeatureCardinalityError } from "../wfs/byId.js"; -import { installZodErrorMapFr } from "./zodErrorMapFr.js"; +import { installZodErrorMapFr, issueName } from "./zodErrorMapFr.js"; // Install the FR Zod error map at module load so the very first parse in the // process already emits localized messages. @@ -51,7 +55,7 @@ type ClassifiedToolError = // --- Problem Type Constants --- /** - * Stable RFC7807-like problem type identifiers exposed in `structuredContent`. + * Stable RFC7807-like problem type identifiers, logged as `problem_type`. */ const INVALID_TOOL_PARAMS_TYPE = "urn:geocontext:problem:invalid-tool-params"; const UPSTREAM_INVALID_REQUEST_TYPE = "urn:geocontext:problem:upstream-invalid-request"; @@ -63,24 +67,6 @@ const FEATURE_CARDINALITY_TYPE = "urn:geocontext:problem:feature-cardinality"; // --- Shared Helpers --- -/** - * Returns the most specific string segment from a Zod issue path. - * - * TODO: this is a best-effort heuristic to extract a user-friendly parameter name - * - * @param path Zod issue path. - * @returns Last non-empty string segment, or `undefined`. - */ -function issueName(path: Array) { - for (let index = path.length - 1; index >= 0; index -= 1) { - const segment = path[index]; - if (typeof segment === "string" && segment.length > 0) { - return segment; - } - } - return undefined; -} - /** * Builds a compact end-user summary from normalized validation errors. * @@ -136,9 +122,18 @@ function normalizeZodIssues(error: ZodError): ToolErrorItem[] { } const name = issueName(issue.path); + // `detail` is the caller's only channel, so the parameter name has to live + // inside it. Messages that already name the parameter are left alone: the + // FR map spells some out inline (`Le paramètre 'x' est requis.`), and + // prefixing those would stutter. Anchored at the start so a message merely + // quoting some other field named `x` still gets its own prefix. + const message = issue.message || "Valeur invalide."; + const messageNamesParam = name !== undefined && message.startsWith(`Le paramètre '${name}'`); + const detail = name && !messageNamesParam ? `${name}: ${message}` : message; + errors.push({ code: issue.code, - detail: issue.message || "Valeur invalide.", + detail, ...(name ? { name } : {}), }); } @@ -296,7 +291,7 @@ function buildExecutionProblem(error: unknown): ToolErrorPayload { * Normalizes any runtime error into the shared MCP tool problem contract. * * @param error Unknown runtime error to normalize. - * @returns A stable payload intended for MCP `structuredContent`. + * @returns A stable problem payload whose `detail` is caller-facing. */ export function normalizeToolError(error: unknown): ToolErrorPayload { const classifiedError = classifyToolError(error); diff --git a/src/errors/zodErrorMapFr.ts b/src/errors/zodErrorMapFr.ts index 94fcef3d..fbea28e3 100644 --- a/src/errors/zodErrorMapFr.ts +++ b/src/errors/zodErrorMapFr.ts @@ -25,14 +25,35 @@ let isInstalled = false; // --- Shared Helpers --- -function issueName(path: IssuePath) { - for (let index = path.length - 1; index >= 0; index -= 1) { - const segment = path[index]; - if (typeof segment === "string" && segment.length > 0) { - return segment; +/** + * Builds a user-friendly parameter name from a Zod issue path. + * + * String segments are joined with `.` to keep nested fields unambiguous + * (`bbox.lon`, not a bare `lon` that two objects could both produce). Array + * indices are kept as `[i]` so per-element errors stay distinguishable + * (`tags[0]` vs `tags[1]`) instead of collapsing into identical messages. + * + * @param path Zod issue path. + * @returns Parameter name, or `undefined` when the path is empty. + */ +export function issueName(path: IssuePath) { + let name = ""; + + for (const segment of path) { + if (typeof segment === "number") { + // A leading index has no field to suffix (root-level array): keep it + // standalone rather than dropping it, or every element would collapse + // to the same nameless message. + name += `[${segment}]`; + continue; + } + if (typeof segment !== "string" || segment.length === 0) { + continue; } + name = name.length > 0 ? `${name}.${segment}` : segment; } - return undefined; + + return name.length > 0 ? name : undefined; } function describeExpectedType(expected: string) { diff --git a/test/errors/toolError.test.ts b/test/errors/toolError.test.ts index 62407d7f..0bc6f3ad 100644 --- a/test/errors/toolError.test.ts +++ b/test/errors/toolError.test.ts @@ -76,7 +76,7 @@ describe("Test toolError helper", () => { expect.objectContaining({ name: "typename", code: "too_small", - detail: "le nom du type ne doit pas être vide", + detail: "typename: le nom du type ne doit pas être vide", }), ]), }); @@ -136,4 +136,107 @@ describe("Test toolError helper", () => { detail: "boom", }); }); + it.each([ + ["enum", z.object({ direction: z.enum(["asc", "desc"]) }), { direction: "sideways" }, "direction: "], + ["invalid type", z.object({ lon: z.number() }), { lon: "abc" }, "lon: "], + ["string format", z.object({ site: z.string().url() }), { site: "nope" }, "site: "], + ["out of range", z.object({ lon: z.number().max(180) }), { lon: 600 }, "lon: "], + [ + "nested field", + z.object({ bbox: z.object({ lon: z.number().max(180) }) }), + { bbox: { lon: 999 } }, + "bbox.lon: ", + ], + [ + "custom refinement", + z.object({ a: z.number() }).superRefine((_value, ctx) => { + ctx.addIssue({ code: z.ZodIssueCode.custom, path: ["a"], message: "Valeur incohérente." }); + }), + { a: 1 }, + "a: Valeur incohérente.", + ], + [ + "array element", + z.object({ tags: z.array(z.string()) }), + { tags: ["ok", 5] }, + "tags[1]: ", + ], + ])("should name the offending parameter for %s issues", (_label, schema, input, expectedPrefix) => { + const result = schema.safeParse(input); + + if (result.success) { + throw new Error("expected parse failure"); + } + + expect(normalizeToolError(result.error).detail).toContain(expectedPrefix); + }); + + it("should keep array element errors distinguishable by index", () => { + const result = z.object({ tags: z.array(z.string()) }).safeParse({ tags: [1, 2] }); + + if (result.success) { + throw new Error("expected parse failure"); + } + + const payload = normalizeToolError(result.error); + + expect(payload.errors.map((error) => error.name)).toEqual(["tags[0]", "tags[1]"]); + }); + + it("should keep root-level array elements distinguishable", () => { + const result = z.array(z.string()).safeParse([1, 2]); + + if (result.success) { + throw new Error("expected parse failure"); + } + + const payload = normalizeToolError(result.error); + + // No field name to suffix, but the indices must survive or the dedupe + // would collapse both elements into a single nameless error. + expect(payload.errors.map((error) => error.name)).toEqual(["[0]", "[1]"]); + }); + + it("should still prefix a message that quotes some other parameter", () => { + const schema = z.object({ a: z.string() }).superRefine((_value, ctx) => { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: ["a"], + message: "Doit valoir le paramètre 'a' du parent.", + }); + }); + const result = schema.safeParse({ a: "x" }); + + if (result.success) { + throw new Error("expected parse failure"); + } + + expect(normalizeToolError(result.error).detail).toContain("a: Doit valoir"); + }); + + it("should not repeat a parameter name the message already carries", () => { + const result = z.object({ text: z.string() }).safeParse({}); + + if (result.success) { + throw new Error("expected parse failure"); + } + + const payload = normalizeToolError(result.error); + + expect(payload.detail).toContain("Le paramètre 'text' est requis."); + expect(payload.detail).not.toContain("text: Le paramètre"); + }); + + it("should not repeat a parameter name an unknown-key message already carries", () => { + const result = z.object({ a: z.string() }).strict().safeParse({ a: "x", nope: 1 }); + + if (result.success) { + throw new Error("expected parse failure"); + } + + const payload = normalizeToolError(result.error); + + expect(payload.detail).toContain("Le paramètre 'nope' n'est pas reconnu."); + expect(payload.detail).not.toContain("nope: "); + }); }); diff --git a/test/tools/helpers/errorAssertions.ts b/test/tools/helpers/errorAssertions.ts index 5aa2382f..324bd992 100644 --- a/test/tools/helpers/errorAssertions.ts +++ b/test/tools/helpers/errorAssertions.ts @@ -35,5 +35,8 @@ export function expectErrorText(response: ToolResponse) { * @param response Tool response expected to be a validation error. */ export function expectInvalidLon(response: ToolResponse) { - expect(expectErrorText(response)).toContain("Paramètres invalides"); + const text = expectErrorText(response); + expect(text).toContain("Paramètres invalides"); + // `detail` is the only channel left, so it must still name the bad param. + expect(text).toContain("lon"); } From 5ea399321bc41333cd4c95f7189b474bef59fbba Mon Sep 17 00:00:00 2001 From: Lionel Zoubritzky Date: Fri, 21 Aug 2026 18:25:14 +0200 Subject: [PATCH 3/4] fix: keep truncated validation summaries actionable --- src/errors/toolError.ts | 47 ++++++++++++++++++++++++++---- test/errors/toolError.test.ts | 55 +++++++++++++++++++++++++++++++++++ 2 files changed, 96 insertions(+), 6 deletions(-) diff --git a/src/errors/toolError.ts b/src/errors/toolError.ts index a6bf22cf..072c1452 100644 --- a/src/errors/toolError.ts +++ b/src/errors/toolError.ts @@ -67,9 +67,20 @@ const FEATURE_CARDINALITY_TYPE = "urn:geocontext:problem:feature-cardinality"; // --- Shared Helpers --- +/** + * Validation messages spelled out in full before the summary elides them. + * + * Five keeps the common "whole call is malformed" case fully detailed while + * bounding the summary to a length a caller can still read at a glance. + */ +const MAX_DETAILED_VALIDATION_ERRORS = 5; + /** * Builds a compact end-user summary from normalized validation errors. * + * Elided messages still list their parameter names, so the caller can fix + * every bad input in one round-trip instead of discovering them one at a time. + * * @param errors Normalized validation errors. * @returns A short, localized validation summary. */ @@ -78,10 +89,21 @@ function summarizeValidationDetail(errors: ToolErrorItem[]) { return "Un ou plusieurs paramètres fournis à l'outil sont invalides."; } - const details = errors.map((error) => error.detail); - const firstDetails = details.slice(0, 3).join(" "); - const suffix = details.length > 3 ? " (et d'autres erreurs)." : ""; - return `Paramètres invalides : ${firstDetails}${suffix}`; + const shown = errors.slice(0, MAX_DETAILED_VALIDATION_ERRORS); + const omitted = errors.slice(MAX_DETAILED_VALIDATION_ERRORS); + const summary = `Paramètres invalides : ${shown.map((error) => error.detail).join(" ")}`; + + if (omitted.length === 0) { + return summary; + } + + const omittedNames = [ + ...new Set(omitted.map((error) => error.name).filter((name): name is string => Boolean(name))), + ]; + const suffix = omittedNames.length > 0 + ? ` (et ${omitted.length} autre(s) erreur(s) sur : ${omittedNames.join(", ")}).` + : ` (et ${omitted.length} autre(s) erreur(s)).`; + return `${summary}${suffix}`; } /** @@ -107,12 +129,25 @@ function toSnakeCase(value: string) { */ function normalizeZodIssues(error: ZodError): ToolErrorItem[] { const errors: ToolErrorItem[] = []; + // A single field can raise the same wording twice (two `ctx.addIssue` calls + // in one refinement); repeating it verbatim only adds noise. Keyed on name + // too, so per-element array errors stay separate. + const seen = new Set(); + + const push = (item: ToolErrorItem) => { + const key = `${item.name ?? ""}\u0000${item.detail}`; + if (seen.has(key)) { + return; + } + seen.add(key); + errors.push(item); + }; for (const issue of error.issues) { if (issue.code === "unrecognized_keys") { const keys = issue.keys.length > 0 ? issue.keys : [""]; for (const key of keys) { - errors.push({ + push({ code: "unknown_parameter", detail: `Le paramètre '${key}' n'est pas reconnu.`, name: key, @@ -131,7 +166,7 @@ function normalizeZodIssues(error: ZodError): ToolErrorItem[] { const messageNamesParam = name !== undefined && message.startsWith(`Le paramètre '${name}'`); const detail = name && !messageNamesParam ? `${name}: ${message}` : message; - errors.push({ + push({ code: issue.code, detail, ...(name ? { name } : {}), diff --git a/test/errors/toolError.test.ts b/test/errors/toolError.test.ts index 0bc6f3ad..7e22837b 100644 --- a/test/errors/toolError.test.ts +++ b/test/errors/toolError.test.ts @@ -239,4 +239,59 @@ describe("Test toolError helper", () => { expect(payload.detail).toContain("Le paramètre 'nope' n'est pas reconnu."); expect(payload.detail).not.toContain("nope: "); }); + it("should name every omitted parameter when truncating the summary", () => { + const keys = ["a", "b", "c", "d", "e", "f", "g"]; + const shape = Object.fromEntries(keys.map((key) => [key, z.number().max(1)])); + const input = Object.fromEntries(keys.map((key) => [key, 9])); + const result = z.object(shape).safeParse(input); + + if (result.success) { + throw new Error("expected parse failure"); + } + + const payload = normalizeToolError(result.error); + + // The first five are spelled out, the rest are named but not detailed. + expect(payload.detail).toContain("a: La valeur doit être au plus 1."); + expect(payload.detail).toContain("e: La valeur doit être au plus 1."); + expect(payload.detail).toContain("(et 2 autre(s) erreur(s) sur : f, g)."); + }); + + it("should not truncate when the summary fits", () => { + const result = z.object({ a: z.number().max(1), b: z.number().max(1) }) + .safeParse({ a: 9, b: 9 }); + + if (result.success) { + throw new Error("expected parse failure"); + } + + expect(normalizeToolError(result.error).detail).not.toContain("autre(s) erreur(s)"); + }); + + it("should drop duplicate messages that share a parameter name", () => { + const schema = z.object({ + a: z.number().superRefine((_value, ctx) => { + ctx.addIssue({ code: z.ZodIssueCode.custom, message: "Valeur incohérente." }); + ctx.addIssue({ code: z.ZodIssueCode.custom, message: "Valeur incohérente." }); + }), + }); + const result = schema.safeParse({ a: 1 }); + + if (result.success) { + throw new Error("expected parse failure"); + } + + expect(normalizeToolError(result.error).errors).toHaveLength(1); + }); + + it("should keep same-wording errors on different parameters", () => { + const result = z.object({ tags: z.array(z.string()) }).safeParse({ tags: [1, 2] }); + + if (result.success) { + throw new Error("expected parse failure"); + } + + // Identical wording, distinct parameters: the dedupe must not merge these. + expect(normalizeToolError(result.error).errors).toHaveLength(2); + }); }); From cb629087acdb42e0c3829d97d37ee6ea042b2de1 Mon Sep 17 00:00:00 2001 From: Lionel Zoubritzky Date: Thu, 1 Oct 2026 16:40:26 +0200 Subject: [PATCH 4/4] test: include value range check in expectInvalidLon --- test/tools/helpers/errorAssertions.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/tools/helpers/errorAssertions.ts b/test/tools/helpers/errorAssertions.ts index 324bd992..b19e7203 100644 --- a/test/tools/helpers/errorAssertions.ts +++ b/test/tools/helpers/errorAssertions.ts @@ -37,6 +37,6 @@ export function expectErrorText(response: ToolResponse) { export function expectInvalidLon(response: ToolResponse) { const text = expectErrorText(response); expect(text).toContain("Paramètres invalides"); - // `detail` is the only channel left, so it must still name the bad param. - expect(text).toContain("lon"); + // `detail` is the only channel left: pin param name and `too_big` wording. + expect(text).toContain("lon: La valeur doit être au plus 180."); }