Skip to content

Commit c8c5442

Browse files
committed
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>
1 parent f31589a commit c8c5442

4 files changed

Lines changed: 75 additions & 1 deletion

File tree

‎clients/web/src/test/core/jsonUtils.test.ts‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -215,6 +215,36 @@ describe("JSON Utils", () => {
215215
});
216216
});
217217

218+
it("sends a non-string discriminator as its typed constant (#2123)", () => {
219+
const numericallyPinned: Tool = {
220+
name: "numerically-pinned",
221+
inputSchema: {
222+
type: "object",
223+
oneOf: [
224+
{
225+
type: "object",
226+
properties: { kind: { const: 1 }, a: { type: "string" } },
227+
required: ["kind"],
228+
},
229+
{
230+
type: "object",
231+
properties: { kind: { const: 2 }, b: { type: "string" } },
232+
required: ["kind"],
233+
},
234+
],
235+
},
236+
};
237+
// `kind=2` selects the second branch, which then rejects `"2"` — the
238+
// schema's own typed value is what goes on the wire.
239+
expect(convertToolParameters(numericallyPinned, { kind: "2" })).toEqual({
240+
kind: 2,
241+
});
242+
// Text that matches no constant is the user's input and is left alone.
243+
expect(convertToolParameters(numericallyPinned, { kind: "9" })).toEqual({
244+
kind: "9",
245+
});
246+
});
247+
218248
it("leaves an ambiguously typed argument uncoerced (#2123)", () => {
219249
const ambiguous: Tool = {
220250
name: "ambiguous",

‎clients/web/src/test/core/nullableUnion.test.ts‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -759,6 +759,15 @@ describe("admitsNull", () => {
759759
);
760760
});
761761

762+
it("admits null when a sibling branch constrains nothing", () => {
763+
// `{}` is the equivalent of `true` — it admits every value, null among
764+
// them — so the pinned null stays reachable and its field submittable.
765+
expect(admitsNull({ const: null, anyOf: [{}] })).toBe(true);
766+
expect(admitsNull({ const: null, anyOf: [{ title: "anything" }] })).toBe(
767+
true,
768+
);
769+
});
770+
762771
it("does not override a sibling that rejects null", () => {
763772
// `const` is conjunctive with its siblings, not an override: both of
764773
// these reject every value, so claiming nullability would let the

‎core/json/jsonUtils.ts‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -264,7 +264,15 @@ export function convertToolParameters(
264264
for (const [key, value] of Object.entries(params)) {
265265
const paramSchema = properties[key] as ParameterSchema | undefined;
266266

267-
if (paramSchema) {
267+
// A `const` the supplied text names is sent as the schema's own typed
268+
// value, not as the text: a branch pinned to `const: 2` is selected by
269+
// `kind=2` and would otherwise be sent `"2"`, which that same branch
270+
// rejects. Only an exact match is substituted — anything else is the
271+
// user's input and is left alone.
272+
const pinned = (paramSchema as { const?: unknown } | undefined)?.const;
273+
if (pinned !== undefined && String(pinned) === value) {
274+
result[key] = pinned as JsonValue;
275+
} else if (paramSchema) {
268276
result[key] = convertParameterValue(value, paramSchema);
269277
} else {
270278
result[key] = value;

‎core/json/nullableUnion.ts‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -427,6 +427,28 @@ export function admitsNull(schema: NullableUnionSchema): boolean {
427427
return anyOfAdmitsNull(schema);
428428
}
429429

430+
/**
431+
* Keywords that annotate rather than constrain — a schema carrying only these
432+
* (or nothing at all) is the equivalent of `true`.
433+
*/
434+
const ANNOTATION_ONLY_KEYWORDS = new Set([
435+
"title",
436+
"description",
437+
"examples",
438+
"default",
439+
"deprecated",
440+
"readOnly",
441+
"writeOnly",
442+
"$comment",
443+
]);
444+
445+
/** Whether a schema states no assertion at all. */
446+
function constrainsNothing(schema: NullableUnionSchema): boolean {
447+
return Object.keys(schema).every((keyword) =>
448+
ANNOTATION_ONLY_KEYWORDS.has(keyword),
449+
);
450+
}
451+
430452
/** Whether some `anyOf` branch is one that admits `null`. */
431453
function anyOfAdmitsNull(schema: NullableUnionSchema): boolean {
432454
const branches = schema.anyOf;
@@ -440,6 +462,11 @@ function anyOfAdmitsNull(schema: NullableUnionSchema): boolean {
440462
// a composition-free schema, which `admitsNull` answers without reaching
441463
// back into this function.
442464
if (hasUnevaluatedComposition(branchSchema)) return false;
465+
// A branch that constrains nothing admits every value, `null` among them.
466+
// `admitsNull` cannot say so on its own — an unconstrained schema tells it
467+
// nothing either way, and it declines rather than guesses — but here the
468+
// question is only whether this alternative leaves null on the table.
469+
if (constrainsNothing(branchSchema)) return true;
443470
// Asked through `admitsNull` rather than by testing `type` alone, so a
444471
// branch spelling its nullability another way — `{ const: null }`,
445472
// `{ nullable: true }` — is recognized the same way the wrapper's own

0 commit comments

Comments
 (0)