fix: address Copilot review round 27 — unconstrained null branches, typed discriminators
- anyOfAdmitsNull recognizes a branch that constrains nothing (`{}`, or
annotations only): it admits every value, null among them, so a field pinned
to `const: null` beside such a union stays submittable. `admitsNull`
cannot say that on its own — an unconstrained schema tells it nothing either
way and it declines rather than guesses — but here the question is narrower.
- convertToolParameters sends a matched `const` as the schema's own typed
value: `kind=2` selects a branch pinned to `const: 2` and was then sent
`"2"`, which that same branch rejects. Only an exact match is
substituted; anything else is the user's input and is left alone.
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
f31589a8f9
commit
c8c5442dcf
4 changed files with 75 additions and 1 deletions
|
|
@ -215,6 +215,36 @@ describe("JSON Utils", () => {
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("sends a non-string discriminator as its typed constant (#2123)", () => {
|
||||||
|
const numericallyPinned: Tool = {
|
||||||
|
name: "numerically-pinned",
|
||||||
|
inputSchema: {
|
||||||
|
type: "object",
|
||||||
|
oneOf: [
|
||||||
|
{
|
||||||
|
type: "object",
|
||||||
|
properties: { kind: { const: 1 }, a: { type: "string" } },
|
||||||
|
required: ["kind"],
|
||||||
|
},
|
||||||
|
{
|
||||||
|
type: "object",
|
||||||
|
properties: { kind: { const: 2 }, b: { type: "string" } },
|
||||||
|
required: ["kind"],
|
||||||
|
},
|
||||||
|
],
|
||||||
|
},
|
||||||
|
};
|
||||||
|
// `kind=2` selects the second branch, which then rejects `"2"` — the
|
||||||
|
// schema's own typed value is what goes on the wire.
|
||||||
|
expect(convertToolParameters(numericallyPinned, { kind: "2" })).toEqual({
|
||||||
|
kind: 2,
|
||||||
|
});
|
||||||
|
// Text that matches no constant is the user's input and is left alone.
|
||||||
|
expect(convertToolParameters(numericallyPinned, { kind: "9" })).toEqual({
|
||||||
|
kind: "9",
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
it("leaves an ambiguously typed argument uncoerced (#2123)", () => {
|
it("leaves an ambiguously typed argument uncoerced (#2123)", () => {
|
||||||
const ambiguous: Tool = {
|
const ambiguous: Tool = {
|
||||||
name: "ambiguous",
|
name: "ambiguous",
|
||||||
|
|
|
||||||
|
|
@ -759,6 +759,15 @@ describe("admitsNull", () => {
|
||||||
);
|
);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("admits null when a sibling branch constrains nothing", () => {
|
||||||
|
// `{}` is the equivalent of `true` — it admits every value, null among
|
||||||
|
// them — so the pinned null stays reachable and its field submittable.
|
||||||
|
expect(admitsNull({ const: null, anyOf: [{}] })).toBe(true);
|
||||||
|
expect(admitsNull({ const: null, anyOf: [{ title: "anything" }] })).toBe(
|
||||||
|
true,
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
it("does not override a sibling that rejects null", () => {
|
it("does not override a sibling that rejects null", () => {
|
||||||
// `const` is conjunctive with its siblings, not an override: both of
|
// `const` is conjunctive with its siblings, not an override: both of
|
||||||
// these reject every value, so claiming nullability would let the
|
// these reject every value, so claiming nullability would let the
|
||||||
|
|
|
||||||
|
|
@ -264,7 +264,15 @@ export function convertToolParameters(
|
||||||
for (const [key, value] of Object.entries(params)) {
|
for (const [key, value] of Object.entries(params)) {
|
||||||
const paramSchema = properties[key] as ParameterSchema | undefined;
|
const paramSchema = properties[key] as ParameterSchema | undefined;
|
||||||
|
|
||||||
if (paramSchema) {
|
// 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
|
||||||
|
// `kind=2` and would otherwise be sent `"2"`, which that same branch
|
||||||
|
// 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);
|
result[key] = convertParameterValue(value, paramSchema);
|
||||||
} else {
|
} else {
|
||||||
result[key] = value;
|
result[key] = value;
|
||||||
|
|
|
||||||
|
|
@ -427,6 +427,28 @@ export function admitsNull(schema: NullableUnionSchema): boolean {
|
||||||
return anyOfAdmitsNull(schema);
|
return anyOfAdmitsNull(schema);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Keywords that annotate rather than constrain — a schema carrying only these
|
||||||
|
* (or nothing at all) is the equivalent of `true`.
|
||||||
|
*/
|
||||||
|
const ANNOTATION_ONLY_KEYWORDS = new Set([
|
||||||
|
"title",
|
||||||
|
"description",
|
||||||
|
"examples",
|
||||||
|
"default",
|
||||||
|
"deprecated",
|
||||||
|
"readOnly",
|
||||||
|
"writeOnly",
|
||||||
|
"$comment",
|
||||||
|
]);
|
||||||
|
|
||||||
|
/** Whether a schema states no assertion at all. */
|
||||||
|
function constrainsNothing(schema: NullableUnionSchema): boolean {
|
||||||
|
return Object.keys(schema).every((keyword) =>
|
||||||
|
ANNOTATION_ONLY_KEYWORDS.has(keyword),
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
/** Whether some `anyOf` branch is one that admits `null`. */
|
/** Whether some `anyOf` branch is one that admits `null`. */
|
||||||
function anyOfAdmitsNull(schema: NullableUnionSchema): boolean {
|
function anyOfAdmitsNull(schema: NullableUnionSchema): boolean {
|
||||||
const branches = schema.anyOf;
|
const branches = schema.anyOf;
|
||||||
|
|
@ -440,6 +462,11 @@ function anyOfAdmitsNull(schema: NullableUnionSchema): boolean {
|
||||||
// a composition-free schema, which `admitsNull` answers without reaching
|
// a composition-free schema, which `admitsNull` answers without reaching
|
||||||
// back into this function.
|
// back into this function.
|
||||||
if (hasUnevaluatedComposition(branchSchema)) return false;
|
if (hasUnevaluatedComposition(branchSchema)) return false;
|
||||||
|
// A branch that constrains nothing admits every value, `null` among them.
|
||||||
|
// `admitsNull` cannot say so on its own — an unconstrained schema tells it
|
||||||
|
// nothing either way, and it declines rather than guesses — but here the
|
||||||
|
// question is only whether this alternative leaves null on the table.
|
||||||
|
if (constrainsNothing(branchSchema)) return true;
|
||||||
// Asked through `admitsNull` rather than by testing `type` alone, so a
|
// Asked through `admitsNull` rather than by testing `type` alone, so a
|
||||||
// branch spelling its nullability another way — `{ const: null }`,
|
// branch spelling its nullability another way — `{ const: null }`,
|
||||||
// `{ nullable: true }` — is recognized the same way the wrapper's own
|
// `{ nullable: true }` — is recognized the same way the wrapper's own
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue