fix: address Copilot review round 17 — shared branch narrowing, null admissibility
- The name-based narrowing is extracted as `narrowBySuppliedNames` and used by the CLI coercion as well as the form: an undiscriminated union whose branches type a shared name differently was losing the coercion for every argument, including the ones that identify the branch unambiguously. The type-agreement fallback now also polls only the const-compatible branches. - TUI missingRequiredFields accepts `null` only where the property admits it, matching the web gate: branch fields are rendered optional, so a `default: null` on a non-nullable required field reached the check and the call went out with a value the schema rejects. Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
7792ad71e9
commit
95030cfdb6
2 changed files with 117 additions and 22 deletions
|
|
@ -1,5 +1,10 @@
|
|||
import type { Tool } from "@modelcontextprotocol/client";
|
||||
import { resolveRootUnion } from "./rootUnion.js";
|
||||
import {
|
||||
narrowBySuppliedNames,
|
||||
resolveRootUnion,
|
||||
type RootUnionBranch,
|
||||
type RootUnionSchema,
|
||||
} from "./rootUnion.js";
|
||||
|
||||
/**
|
||||
* JSON value type used across the inspector project
|
||||
|
|
@ -146,23 +151,34 @@ export function convertParameterValue(
|
|||
* `value=true` into `Number("true")`, i.e. `NaN`. Passing the raw string
|
||||
* through is what this function did for every argument before it existed.
|
||||
*/
|
||||
function coercionProperties(
|
||||
base: { properties?: Record<string, unknown> },
|
||||
branches: {
|
||||
schema: { properties?: Record<string, unknown> };
|
||||
declaredFields: string[];
|
||||
}[],
|
||||
function coercionProperties<T extends RootUnionSchema>(
|
||||
base: T,
|
||||
branches: RootUnionBranch<T>[],
|
||||
params: Record<string, string>,
|
||||
): Record<string, unknown> {
|
||||
if (branches.length === 0) {
|
||||
return { ...base.properties };
|
||||
}
|
||||
|
||||
const matching = branches.filter((branch) =>
|
||||
matchesConstants(branch.schema.properties ?? {}, params),
|
||||
// Which branch the call means, from the constants it supplies and then — when
|
||||
// those leave more than one standing — from the argument NAMES it supplies,
|
||||
// through the same narrowing the form uses. Without the second step an
|
||||
// undiscriminated union whose branches type a shared name differently loses
|
||||
// the coercion for every argument in it, including the ones that identify
|
||||
// the branch unambiguously.
|
||||
const candidates = branches
|
||||
.map((branch, index) => ({ branch, index }))
|
||||
.filter(({ branch }) =>
|
||||
matchesConstants(branch.schema.properties ?? {}, params),
|
||||
)
|
||||
.map(({ index }) => index);
|
||||
const selected = narrowBySuppliedNames(
|
||||
branches,
|
||||
candidates,
|
||||
Object.keys(params),
|
||||
);
|
||||
if (matching.length === 1) {
|
||||
return { ...matching[0].schema.properties };
|
||||
if (selected !== null) {
|
||||
return { ...branches[selected]!.schema.properties };
|
||||
}
|
||||
|
||||
// `hasOwn`/`fromEntries` rather than `in`/assignment throughout: `properties`
|
||||
|
|
@ -171,12 +187,14 @@ function coercionProperties(
|
|||
const properties: Record<string, unknown> = Object.fromEntries(
|
||||
Object.entries(base.properties ?? {}),
|
||||
);
|
||||
for (const name of new Set(branches.flatMap((b) => b.declaredFields))) {
|
||||
const pool =
|
||||
candidates.length > 0 ? candidates.map((i) => branches[i]!) : branches;
|
||||
for (const name of new Set(pool.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,
|
||||
// disagreeing vote. Read through the merged schema so a branch's
|
||||
// specialization of a root property carries the root's keywords too.
|
||||
const declarations = branches
|
||||
const declarations = pool
|
||||
.filter((branch) => branch.declaredFields.includes(name))
|
||||
.map((branch) => branch.schema.properties?.[name])
|
||||
// A malformed declaration (`properties: { x: null }`) is not a vote about
|
||||
|
|
|
|||
|
|
@ -713,23 +713,44 @@ export function selectBranchIndex<T extends RootUnionSchema>(
|
|||
|
||||
// One branch's discriminator matched and no other's did — the plain case.
|
||||
if (agreeing.length === 1) return agreeing[0];
|
||||
if (candidates.length === 1) return candidates[0];
|
||||
if (candidates.length === 0 || Object.keys(values).length === 0) return null;
|
||||
|
||||
// Several branches remain — they share the constant that was supplied, or
|
||||
// none was. The values still belong to a shape, so keep looking among the
|
||||
// candidates: first the branch whose own required fields they supply.
|
||||
// candidates by the names that were supplied.
|
||||
return narrowBySuppliedNames(
|
||||
branches,
|
||||
candidates,
|
||||
Object.keys(values).filter(supplied),
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* Choose among candidate branches using only *which* argument names were
|
||||
* supplied — first the branch whose own required fields they cover, then a name
|
||||
* only one candidate declares.
|
||||
*
|
||||
* Split out because two callers need the same answer from different evidence:
|
||||
* the form holds typed values, while the CLI holds strings it has not coerced
|
||||
* yet, and a name is a name in both. A name more than one candidate declares is
|
||||
* ambiguous and says nothing.
|
||||
*/
|
||||
export function narrowBySuppliedNames<T extends RootUnionSchema>(
|
||||
branches: RootUnionBranch<T>[],
|
||||
candidates: number[],
|
||||
suppliedNames: string[],
|
||||
): number | null {
|
||||
if (candidates.length === 1) return candidates[0];
|
||||
if (candidates.length === 0 || suppliedNames.length === 0) return null;
|
||||
const supplied = new Set(suppliedNames);
|
||||
|
||||
const satisfied = candidates.filter((index) => {
|
||||
const branch = branches[index]!;
|
||||
const required = branch.schema.required ?? [];
|
||||
const own = required.filter((name) => branch.declaredFields.includes(name));
|
||||
return own.length > 0 && own.every(supplied);
|
||||
return own.length > 0 && own.every((name) => supplied.has(name));
|
||||
});
|
||||
if (satisfied.length === 1) return satisfied[0];
|
||||
|
||||
// Then a name only ONE candidate declares: supplying `phone` where only the
|
||||
// SMS branch declares it names that shape as clearly as a discriminator
|
||||
// would. A name more than one declares is ambiguous and says nothing.
|
||||
const exclusiveTo = new Map<string, number>();
|
||||
for (const index of candidates) {
|
||||
for (const name of branches[index]!.declaredFields) {
|
||||
|
|
@ -737,8 +758,7 @@ export function selectBranchIndex<T extends RootUnionSchema>(
|
|||
}
|
||||
}
|
||||
const named = new Set(
|
||||
Object.keys(values)
|
||||
.filter(supplied)
|
||||
suppliedNames
|
||||
.map((name) => exclusiveTo.get(name))
|
||||
.filter((index): index is number => index !== undefined && index >= 0),
|
||||
);
|
||||
|
|
|
|||
Loading…
Reference in a new issue