fix: address Copilot review round 6 — additionalProperties, annotations, prototype-safe records

- rootUnion: decline a union that adds fields under a restrictive root
  `additionalProperties`. The keyword constrains what its SIBLING
  `properties` does not name, so the original schema admits none of the
  branch fields; flattening moves them beside the keyword, where they read as
  allowed and the form would submit what the schema forbids.
- `default` joins the annotation set: it constrains nothing, so two
  declarations suggesting different initial values no longer conflict.
- Property records are read with `Object.hasOwn` and built with
  `fromEntries`/`defineProperty`. `properties` is a JSON record, so
  `constructor` is a legal argument name that `in` reported as a collision
  the root never made, and `__proto__` is one that assignment would drop
  into the legacy prototype setter instead of keeping as a field.
- SchemaForm: a value survives a branch switch unless BOTH branches declare
  the name. A root argument the outgoing branch merely specialized is still a
  root argument in a branch that inherits it, so dropping it erased a valid
  value.

Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-08-26 02:56:55 -04:00
commit c15e30414f
2 changed files with 181 additions and 17 deletions

View file

@ -165,7 +165,12 @@ function coercionProperties(
return { ...matching[0].schema.properties };
}
const properties: Record<string, unknown> = { ...base.properties };
// `hasOwn`/`fromEntries` rather than `in`/assignment throughout: `properties`
// is a JSON record, so `constructor` and `__proto__` are legal argument names
// that the prototype chain and the legacy setter would otherwise mishandle.
const properties: Record<string, unknown> = Object.fromEntries(
Object.entries(base.properties ?? {}),
);
for (const name of new Set(branches.flatMap((b) => b.declaredFields))) {
// Only the branches that *declare* the name have an opinion about it — a
// branch that merely inherited the root's declaration is not a second,
@ -176,7 +181,12 @@ function coercionProperties(
.map((branch) => branch.schema.properties?.[name]);
const types = new Set(declarations.map((schema) => typeNameOf(schema)));
if (types.size === 1) {
properties[name] = declarations[0];
Object.defineProperty(properties, name, {
value: declarations[0],
writable: true,
enumerable: true,
configurable: true,
});
} else {
delete properties[name];
}

View file

@ -42,6 +42,12 @@ export interface RootUnionSchema {
* own repro does). Read only to *label* a branch — never to validate.
*/
discriminator?: { propertyName?: string };
/**
* Read only to *decline*: `additionalProperties` constrains the names its
* **sibling** `properties` does not list, so a restrictive one at the root
* rejects every field a branch adds. See {@link resolveRootUnion}.
*/
additionalProperties?: unknown;
/**
* Read only to *decline* a member: its referent is not resolved here, so a
* `$ref` member's constraints are unknown rather than absent.
@ -160,6 +166,9 @@ const ANNOTATION_KEYWORDS = new Set([
"readOnly",
"writeOnly",
"$comment",
// An annotation too: a suggested initial value constrains nothing, so two
// declarations suggesting different ones still accept the same values.
"default",
]);
/** Structural equality, via canonical JSON — enough for schema keyword values. */
@ -254,7 +263,11 @@ function conflictsWithBase(
const branchProperties = propertiesOf(branch) ?? {};
return Object.entries(branchProperties).some(
([name, branchProperty]) =>
name in baseProperties && conflicts(baseProperties[name], branchProperty),
// `hasOwn`, not `in`: `properties` is a JSON record, so `constructor` and
// `toString` are legal argument names that `in` would find on
// `Object.prototype` and report as collisions the root never declared.
Object.hasOwn(baseProperties, name) &&
conflicts(baseProperties[name], branchProperty),
);
}
@ -297,13 +310,19 @@ function mergeBranch<T extends RootUnionSchema>(
): ResolvedSchema<T> {
const baseProperties = propertiesOf(base) ?? {};
const branchProperties = propertiesOf(branch) ?? {};
const properties: Record<string, unknown> = { ...baseProperties };
for (const [name, branchProperty] of Object.entries(branchProperties)) {
properties[name] =
name in baseProperties
// Built through `fromEntries` rather than by assignment: a property named
// `__proto__` is a legal argument name, and assigning it would invoke the
// legacy prototype setter instead of creating an own property — losing the
// field entirely. `hasOwn` for the same reason `conflictsWithBase` uses it.
const properties: Record<string, unknown> = Object.fromEntries([
...Object.entries(baseProperties),
...Object.entries(branchProperties).map(([name, branchProperty]) => [
name,
Object.hasOwn(baseProperties, name)
? mergeProperty(baseProperties[name], branchProperty)
: branchProperty;
}
: branchProperty,
]),
]);
const required = [
...(base.required ?? []),
...(branch.required ?? []).filter(
@ -441,6 +460,27 @@ export function resolveRootUnion<T extends RootUnionSchema>(
return { base, branches: [] };
}
// `additionalProperties` applies to whatever its **sibling** `properties`
// does not name, so a restrictive one at the root rejects every field the
// branches add — the original schema admits none of them. Flattening moves
// those fields *beside* the keyword, where they would read as allowed, so a
// form built from it would submit what the schema forbids.
const additional = base.additionalProperties;
const restrictsAdditional =
additional === false ||
(typeof additional === "object" && additional !== null);
const baseNames = propertiesOf(base) ?? {};
if (
restrictsAdditional &&
branches.some((branch) =>
Object.keys(propertiesOf(branch as RootUnionSchema) ?? {}).some(
(name) => !Object.hasOwn(baseNames, name),
),
)
) {
return { base, branches: [] };
}
const discriminatorProperty = schema.discriminator?.propertyName;
return {
base,