fix: address Copilot review round 20 — malformed applicators, const-null under anyOf
- rootUnion reads every composition keyword through `membersOf` and every
`required` through `requiredOf`. These schemas describe the wire and each
member arrives as `unknown`, so `anyOf: {}` or `required: "x"` really
can arrive — and reading them as lists threw, taking all three clients down
rather than declining one malformed tool. Both web and TUI required-field
checks are guarded the same way.
- admitsNull evaluates a sibling `anyOf` beside a `const: null` instead of
refusing outright: the union decides in both directions, and
`{ const: null, anyOf: [{ type: "null" }] }` admits null as plainly as an
all-string union rejects it. Refusing it left a read-only required field
seeded with the only value it accepts and submit permanently disabled.
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
44b2ccd257
commit
d334710d6f
4 changed files with 102 additions and 36 deletions
|
|
@ -322,7 +322,9 @@ function applyConstants<T>(
|
|||
function buildFields(schema: JsonSchemaObject): FormField[] {
|
||||
const fields: FormField[] = [];
|
||||
const properties = schema.properties || {};
|
||||
const required = schema.required || [];
|
||||
// `Array.isArray`, not `|| []`: a nonconforming server can send
|
||||
// `required: "x"`, and `.includes` on a string silently matches substrings.
|
||||
const required = Array.isArray(schema.required) ? schema.required : [];
|
||||
|
||||
for (const [key, prop] of Object.entries(properties)) {
|
||||
// `properties` values are `unknown` (the SDK schema admits anything), so
|
||||
|
|
@ -463,7 +465,8 @@ export function missingRequiredFields(
|
|||
? base
|
||||
: branches[selectedBranchIndex(base, branches, rawValues)]!.schema;
|
||||
const properties = effective.properties ?? {};
|
||||
return (effective.required ?? []).filter((name) => {
|
||||
const required = Array.isArray(effective.required) ? effective.required : [];
|
||||
return required.filter((name) => {
|
||||
// `hasOwn` first: an argument legally named `constructor` would otherwise
|
||||
// resolve to the inherited one and read as supplied, and the call would go
|
||||
// out without it.
|
||||
|
|
|
|||
|
|
@ -745,6 +745,12 @@ describe("admitsNull", () => {
|
|||
);
|
||||
});
|
||||
|
||||
it("admits null when a sibling anyOf branch does", () => {
|
||||
// The union decides in both directions: an all-string one rejects null,
|
||||
// and a null branch admits it just as plainly.
|
||||
expect(admitsNull({ const: null, anyOf: [{ type: "null" }] })).toBe(true);
|
||||
});
|
||||
|
||||
it("does not override a sibling that rejects null", () => {
|
||||
// `const` is conjunctive with its siblings, not an override: both of
|
||||
// these reject every value, so claiming nullability would let the
|
||||
|
|
|
|||
|
|
@ -664,6 +664,35 @@ describe("resolveRootUnion", () => {
|
|||
expect(branches).toEqual([]);
|
||||
});
|
||||
|
||||
it("survives a composition keyword that is not a list", () => {
|
||||
// These schemas describe the wire, and every member arrives as `unknown`
|
||||
// — reading `anyOf: {}` as a list would throw and take all three clients
|
||||
// down rather than declining one malformed tool.
|
||||
const { base, branches } = resolveRootUnion({
|
||||
type: "object",
|
||||
properties: { a: { type: "string" } },
|
||||
anyOf: {} as unknown as unknown[],
|
||||
});
|
||||
expect(branches).toEqual([]);
|
||||
expect(Object.keys(base.properties ?? {})).toEqual(["a"]);
|
||||
});
|
||||
|
||||
it("survives a required that is not a list", () => {
|
||||
const { branches } = resolveRootUnion({
|
||||
type: "object",
|
||||
anyOf: [
|
||||
{
|
||||
type: "object",
|
||||
properties: { a: { type: "string" } },
|
||||
required: "a",
|
||||
},
|
||||
{ type: "object", properties: { b: { type: "string" } } },
|
||||
] as unknown[],
|
||||
});
|
||||
expect(branches).toHaveLength(2);
|
||||
expect(branches[0].schema.required).toBeUndefined();
|
||||
});
|
||||
|
||||
it("declines an empty union", () => {
|
||||
expect(resolveRootUnion({ type: "object", anyOf: [] }).branches).toEqual(
|
||||
[],
|
||||
|
|
|
|||
|
|
@ -284,7 +284,10 @@ function hasMissingIn(
|
|||
schema: InspectorFormSchema,
|
||||
values: Record<string, unknown>,
|
||||
): boolean {
|
||||
const required = schema.required ?? [];
|
||||
// `Array.isArray`, not `?? []`: these schemas describe the wire, and a
|
||||
// nonconforming server can send `required: "x"` — which `.some` would throw
|
||||
// on, taking the whole panel down rather than one malformed tool.
|
||||
const required = Array.isArray(schema.required) ? schema.required : [];
|
||||
const properties = schema.properties ?? {};
|
||||
return required.some((field) => {
|
||||
const value = values[field];
|
||||
|
|
|
|||
Loading…
Reference in a new issue