fix: address Copilot review round 31 — proto-safe results, set-valued types, core keywords
- convertToolParameters defines its result keys rather than assigning them, so a discriminator legally named `__proto__` reaches the call instead of the prototype setter. It also collapses a nullable declaration before dispatching, so `type: ["number","null"]` coerces as a number rather than being sent as text. - typeNameOf sorts an array `type`: JSON Schema reads it as a SET, so `["number","null"]` and `["null","number"]` are one declaration and must not read as branches disagreeing. - nullableUnion's non-asserting keyword set covers `$defs`, `$id`, `$schema`, `$anchor`, `$dynamicAnchor` and `$vocabulary` — they identify or declare rather than assert. `$ref` stays out: it applies whatever it points at. Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
67e7a4ca1a
commit
c9162d0e73
2 changed files with 87 additions and 11 deletions
|
|
@ -1,4 +1,5 @@
|
|||
import type { Tool } from "@modelcontextprotocol/client";
|
||||
import { normalizeNullableUnion } from "./nullableUnion.js";
|
||||
import {
|
||||
narrowBySuppliedNames,
|
||||
resolveRootUnion,
|
||||
|
|
@ -220,8 +221,11 @@ function coercionProperties<T extends RootUnionSchema>(
|
|||
function typeNameOf(schema: unknown): string {
|
||||
if (typeof schema !== "object" || schema === null) return "";
|
||||
const { type } = schema as { type?: unknown };
|
||||
// Sorted: JSON Schema reads an array `type` as a SET, so `["number","null"]`
|
||||
// and `["null","number"]` are the same declaration and must not read as a
|
||||
// disagreement that drops the property from the coercion map.
|
||||
return Array.isArray(type)
|
||||
? type.join(",")
|
||||
? [...type].map(String).sort().join(",")
|
||||
: typeof type === "string"
|
||||
? type
|
||||
: "";
|
||||
|
|
@ -262,7 +266,15 @@ export function convertToolParameters(
|
|||
const { base, branches } = resolveRootUnion(tool.inputSchema ?? {});
|
||||
const properties = coercionProperties(base, branches, params);
|
||||
for (const [key, value] of Object.entries(params)) {
|
||||
const paramSchema = properties[key] as ParameterSchema | undefined;
|
||||
const declared = properties[key];
|
||||
// Collapsed first: a nullable declaration (`type: ["number","null"]`, or an
|
||||
// `anyOf` with a null branch) states its real type on the surviving branch,
|
||||
// and `convertParameterValue` dispatches on a single `type` string — so
|
||||
// without this a nullable number is sent as the string it was typed as.
|
||||
const paramSchema =
|
||||
typeof declared === "object" && declared !== null
|
||||
? (normalizeNullableUnion(declared) as ParameterSchema)
|
||||
: (declared as ParameterSchema | undefined);
|
||||
|
||||
// A `const` the supplied text names is sent as the schema's own typed
|
||||
// value, not as the text: a branch pinned to `const: 2` is selected by
|
||||
|
|
@ -270,13 +282,21 @@ export function convertToolParameters(
|
|||
// rejects. Only an exact match is substituted — anything else is the
|
||||
// user's input and is left alone.
|
||||
const pinned = (paramSchema as { const?: unknown } | undefined)?.const;
|
||||
if (pinned !== undefined && String(pinned) === value) {
|
||||
result[key] = pinned as JsonValue;
|
||||
} else if (paramSchema) {
|
||||
result[key] = convertParameterValue(value, paramSchema);
|
||||
} else {
|
||||
result[key] = value;
|
||||
}
|
||||
const converted =
|
||||
pinned !== undefined && String(pinned) === value
|
||||
? (pinned as JsonValue)
|
||||
: paramSchema
|
||||
? convertParameterValue(value, paramSchema)
|
||||
: value;
|
||||
// `defineProperty`, not assignment: `__proto__` is a legal argument name —
|
||||
// a discriminator can carry it — and assigning it would invoke the legacy
|
||||
// prototype setter instead of putting it in the call.
|
||||
Object.defineProperty(result, key, {
|
||||
value: converted,
|
||||
writable: true,
|
||||
enumerable: true,
|
||||
configurable: true,
|
||||
});
|
||||
}
|
||||
|
||||
return result;
|
||||
|
|
|
|||
|
|
@ -440,6 +440,15 @@ const ANNOTATION_ONLY_KEYWORDS = new Set([
|
|||
"readOnly",
|
||||
"writeOnly",
|
||||
"$comment",
|
||||
// Core keywords that identify or declare rather than assert. `$ref` is
|
||||
// deliberately absent: it applies whatever it points at, which is exactly an
|
||||
// assertion this module cannot see.
|
||||
"$defs",
|
||||
"$id",
|
||||
"$schema",
|
||||
"$anchor",
|
||||
"$dynamicAnchor",
|
||||
"$vocabulary",
|
||||
]);
|
||||
|
||||
/** Whether a schema states no assertion at all. */
|
||||
|
|
|
|||
Loading…
Reference in a new issue