modelcontextprotocol/inspector
Watch
1
0
Fork
You've already forked inspector
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 4

fix: dedupe a repeated name in a group, and name every unmet requirement

Browse source 
Addresses Copilot's round-13 review on #2035. Two suppressed comments, both
real, both in the required-group work from round 7 rather than in the recent
preview rework.

1. `{a,a}` is ONE requirement named twice, but `requiredGroups` stored it as
   a two-name group, so everything downstream read it as "either of these
   will do". The TUI was left with a form it could not submit: its
   `length === 1` test marked `a` optional while ResourceTestModal's guard
   still refused a blank. The web panel offered the useless hint
   "Any one of: a, a". Names are now deduplicated within the expression --
   expansion still emits both occurrences (`x://{a,a}` -> `x://1,1`), which
   RFC 6570 requires.

2. The per-field hint used `find`, so a variable in several shared groups
   advertised only the first. With `{a,b}{b,c}{a,c}`, filling `b` satisfies
   the first two and every visible hint then looks met while Read Resource
   stays disabled on the unmet `{a,c}` -- a requirement with nothing on
   screen pointing at it.

   Fields now hint every shared group they sit in. That alone is not enough,
   because no per-field hint can say WHICH group is still outstanding, so
   the form states it directly: "Still needed: a or c", built from the same
   `unmetRequiredGroups` the submit gate uses, so the message and the button
   cannot disagree. `hasRequiredValues` is no longer called here -- the
   panel derives both from the one list.

npm run ci passes.

Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-08-17 00:33:52 -04:00
commit 1c595e96ee
6 changed files with 120 additions and 9 deletions
Download patch file Download diff file Expand all files Collapse all files

12
clients/tui/__tests__/uriTemplateToForm.test.ts
View file

@ -64,3 +64,15 @@ describe("uriTemplateToForm", () => {
expect(form.sections[0]!.fields).toEqual([]);
});
});
describe("a name repeated inside one expression", () => {
it("is required, not treated as a shared group", () => {
// `{a,a}` is one requirement named twice. Before core deduplicated the
// group, `requiredGroups` returned ["a","a"], so this form's
// `length === 1` test left the field optional while ResourceTestModal's
// submit guard still refused a blank -- an un-submittable form.
const [field] = uriTemplateToForm("x://{a,a}", "T").sections[0].fields;
expect(field.name).toBe("a");
expect(field.required).toBe(true);
});
});

52
clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.test.tsx
View file

@ -266,6 +266,58 @@ describe("ResourceTemplatePanel", () => {
expect(onReadResource).not.toHaveBeenCalled();
});
it("names every unmet group, so a disabled button always has a reason", async () => {
// With `{a,b}{b,c}{a,c}`, filling `b` satisfies the first two groups. A
// per-field hint then looks satisfied while Read Resource stays disabled
// on the third -- so the outstanding requirement is stated form-level.
const user = userEvent.setup();
renderWithMantine(
<ResourceTemplatePanel
template={{ name: "Shared", uriTemplate: "x://{a,b}{b,c}{a,c}" }}
onReadResource={vi.fn()}
/>,
);
expect(screen.getByText(/Still needed:/)).toHaveTextContent(
"Still needed: a or b; b or c; a or c",
);
await user.type(screen.getByLabelText("b"), "1");
expect(screen.getByText(/Still needed:/)).toHaveTextContent(
"Still needed: a or c",
);
expect(
screen.getByRole("button", { name: "Read Resource" }),
).toBeDisabled();
await user.type(screen.getByLabelText("a"), "2");
expect(screen.queryByText(/Still needed:/)).not.toBeInTheDocument();
expect(
screen.getByRole("button", { name: "Read Resource" }),
).toBeEnabled();
});
it("hints every shared group a variable sits in, not just the first", () => {
renderWithMantine(
<ResourceTemplatePanel
template={{ name: "Shared", uriTemplate: "x://{a,b}{a,c}" }}
onReadResource={vi.fn()}
/>,
);
expect(
screen.getByText("Any one of each: a, b; a, c"),
).toBeInTheDocument();
});
it("treats a name repeated in one expression as a plain requirement", () => {
renderWithMantine(
<ResourceTemplatePanel
template={{ name: "Twice", uriTemplate: "x://{a,a}" }}
onReadResource={vi.fn()}
/>,
);
// Not "Any one of: a, a" -- it is one field, required.
expect(screen.queryByText(/Any one of/)).not.toBeInTheDocument();
expect(screen.getByText("Still needed: a")).toBeInTheDocument();
});
it("shows the malformed template unexpanded in the preview", () => {
renderWithMantine(
<ResourceTemplatePanel

39
clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx
View file

@ -16,11 +16,11 @@ import { AnnotationBadge } from "../../elements/AnnotationBadge/AnnotationBadge"
import { CopyButton } from "../../elements/CopyButton/CopyButton";
import {
definedValues,
hasRequiredValues,
previewUriTemplate,
requiredGroups,
templateVariables,
tryExpandUriTemplate,
unmetRequiredGroups,
} from "../../../utils/uriTemplate";
export interface ResourceTemplatePanelProps {
@ -77,6 +77,13 @@ const DescriptionText = Text.withProps({
// value that cannot be encoded -- so a screen-reader user would otherwise meet
// a silently disabled action. An alert is an assertive live region, which is
// what announces text that arrives after first render.
// What is still missing before Read Resource can fire. Dimmed rather than red:
// an incomplete form is the expected starting state, not an error.
const RequirementText = Text.withProps({
size: "sm",
c: "dimmed",
});
const ExpansionErrorText = Text.withProps({
size: "sm",
c: "red",
@ -255,8 +262,11 @@ export function ResourceTemplatePanel({
// untouched field is indistinguishable from a deliberately empty one. That is
// a fact about the form, so the form is what resolves it.
const expansion = tryExpandUriTemplate(uriTemplate, definedValues(variables));
const canSubmit =
expansion.error === undefined && hasRequiredValues(groups, variables);
// Named rather than merely counted, so a disabled Read Resource always has a
// reason on screen. Per-field hints cannot carry this: a variable shared
// across several groups looks satisfied once any one of them is met.
const unmet = unmetRequiredGroups(groups, variables);
const canSubmit = expansion.error === undefined && unmet.length === 0;
function handleSubmit() {
/* v8 ignore next -- unreachable: the button is disabled whenever the
@ -293,15 +303,23 @@ export function ResourceTemplatePanel({
const individuallyRequired = groups.some(
(names) => names.length === 1 && names[0] === varName,
);
const sharedGroup = individuallyRequired
? undefined
: groups.find(
// EVERY shared group this variable sits in, not the first. With
// `{a,b}{b,c}{a,c}`, showing only the first left `b` looking like it
// satisfied everything while Read Resource stayed disabled on the
// unmet `{a,c}` -- a hidden requirement is worse than a wordy hint.
// The form-level "Still needed" line below names what is actually
// outstanding, which is the part a per-field hint cannot express.
const sharedGroups = individuallyRequired
? []
: groups.filter(
(names) => names.length > 1 && names.includes(varName),
);
const description = !required
? "Optional"
: sharedGroup
? `Any one of: ${sharedGroup.join(", ")}`
: sharedGroups.length > 0
? `Any one of${sharedGroups.length > 1 ? " each" : ""}: ${sharedGroups
.map((names) => names.join(", "))
.join("; ")}`
: undefined;
return useAutocomplete ? (
<Autocomplete
@ -344,6 +362,11 @@ export function ResourceTemplatePanel({
{expansion.error !== undefined && (
<ExpansionErrorText>{expansion.error}</ExpansionErrorText>
)}
{expansion.error === undefined && unmet.length > 0 && (
<RequirementText>
Still needed: {unmet.map((names) => names.join(" or ")).join("; ")}
</RequirementText>
)}
<FooterRow>
<Button size="sm" disabled={!canSubmit} onClick={handleSubmit}>
Read Resource

18
clients/web/src/test/core/mcp/uriTemplate.test.ts
View file

@ -352,6 +352,24 @@ describe("requiredGroups / hasRequiredValues", () => {
});
});
describe("a name repeated inside one expression", () => {
// `{a,a}` is one requirement named twice, not two names either of which
// would do. Undeduplicated it read as a shared group everywhere downstream:
// the TUI's `length === 1` test marked `a` optional while its submit guard
// still refused a blank, and the web panel offered "Any one of: a, a".
it("collapses to a single-name required group", () => {
expect(requiredGroups("x://{a,a}")).toEqual([["a"]]);
});
it("still expands both occurrences, which RFC 6570 requires", () => {
expect(expandUriTemplate("x://{a,a}", { a: "1" })).toBe("x://1,1");
});
it("leaves genuinely distinct names alone", () => {
expect(requiredGroups("x://{a,b}")).toEqual([["a", "b"]]);
});
});
describe("expression independence", () => {
it("does not rewrite a later query expression on the own-expansion path", () => {
expect(

1
clients/web/src/utils/uriTemplate.ts
View file

@ -26,6 +26,7 @@ export {
requiredGroups,
templateVariables,
tryExpandUriTemplate,
unmetRequiredGroups,
} from "@inspector/core/mcp/uriTemplate.js";
export type {
TemplateExpansion,

7
core/mcp/uriTemplate.ts
View file

@ -317,7 +317,12 @@ export function requiredGroups(uriTemplate: string): string[][] {
// time on a condition nothing can satisfy, and the "any one of" message
// built from it would name no fields.
if (part.names.length === 0) continue;
groups.push(part.names);
// Deduplicated within the expression: `{a,a}` is one requirement named
// twice, not two names either of which would do. Left as-is it read as a
// shared group everywhere downstream -- the TUI's `length === 1` test
// marked `a` optional while its submit guard still refused a blank, and the
// web panel offered the useless hint "Any one of: a, a".
groups.push([...new Set(part.names)]);
}
return groups;
}