fix: address Copilot review round 25 — names before constants, unrenderable properties

- selectBranchIndex asks the supplied NAMES first and falls back to a lone
  agreeing constant only when they settle nothing. A matching `const` does
  not identify a branch while another candidate leaves that property unpinned:
  `{ kind: "email", phone: "555" }` agrees with an email branch whose
  `address` is missing while satisfying a phone branch outright.
- isOfferable requires every property VALUE to be readable — JSON Schema's
  boolean form is fine, a `null` or an array is not a schema at all and the
  web form dereferences one on the way to choosing a widget. Such a branch is
  declined rather than handed on to crash a tool panel that would otherwise
  have rendered.

Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-08-26 09:30:36 -04:00
commit ab3f8c779a
2 changed files with 97 additions and 20 deletions

View file

@ -291,11 +291,12 @@ describe("JSON Utils", () => {
],
},
};
// A `properties: { broken: null }` entry must not throw, and a branch is
// still identifiable by the discriminator that *is* well-formed.
// A `properties: { broken: null }` entry must not throw. The union is
// declined — a `null` is not a schema, and handing it on would crash a
// renderer — so nothing is coerced and the strings pass through.
expect(
convertToolParameters(malformed, { kind: "a", value: "3" }),
).toEqual({ kind: "a", value: 3 });
).toEqual({ kind: "a", value: "3" });
});
it("falls back to the branch-agreement path when no constant is supplied (#2123)", () => {
@ -342,11 +343,11 @@ describe("JSON Utils", () => {
],
},
};
// The `null` is not a vote about the type, and must not end up standing
// in for one — the surviving declaration is what coerces.
// The `null` makes the whole union unofferable, so nothing is coerced —
// and, the point of the test, nothing throws either.
expect(
convertToolParameters(malformedDeclaration, { count: "3" }),
).toEqual({ count: 3 });
).toEqual({ count: "3" });
});
it("coerces a value whose schema lives on a root allOf branch (#2123)", () => {

View file

@ -748,6 +748,35 @@ describe("resolveRootUnion", () => {
expect(branches).toEqual([]);
});
it("declines a member carrying a property that is not a schema", () => {
// A `null` or an array is not a schema, and the web form dereferences
// one on the way to choosing a widget — so the branch is declined rather
// than handed on to crash a tool panel.
for (const property of [null, [1]] as unknown[]) {
const { branches } = resolveRootUnion({
type: "object",
anyOf: [
EMAIL,
{ type: "object", properties: { broken: property } },
] as unknown[],
});
expect(branches).toEqual([]);
}
});
it("offers a member carrying a boolean property schema", () => {
// JSON Schema's boolean form is legal and answers every keyword lookup
// with `undefined`, so it renders through the JSON fallback harmlessly.
const { branches } = resolveRootUnion({
type: "object",
anyOf: [
{ type: "object", properties: { anything: true } },
{ type: "object", properties: { other: { type: "string" } } },
] as unknown[],
});
expect(branches).toHaveLength(2);
});
it("declines a member whose properties are not an object", () => {
// Members arrive as `unknown`, so this is reachable and must not throw.
expect(
@ -979,6 +1008,33 @@ describe("resolveRootUnion", () => {
);
});
it("prefers the branch the supplied names satisfy over a matching constant", () => {
// `{ kind: "email", phone: "555" }` agrees with the email branch's
// discriminator while missing its `address`, and satisfies the phone
// branch outright — the picker must show the one that can be called.
const mixed = resolveRootUnion({
type: "object",
anyOf: [
{
type: "object",
properties: {
kind: { const: "email" },
address: { type: "string" },
},
required: ["kind", "address"],
},
{
type: "object",
properties: { phone: { type: "string" } },
required: ["phone"],
},
],
}).branches;
expect(selectBranchIndex(mixed, { kind: "email", phone: "555" })).toBe(1);
// …and the constant still decides when the names settle nothing.
expect(selectBranchIndex(mixed, { kind: "email" })).toBe(0);
});
it("reports none when the values identify nothing", () => {
expect(selectBranchIndex(branches, {})).toBeNull();
expect(selectBranchIndex(branches, { kind: "other" })).toBeNull();