From f5337609dc2436576c19bd804c487c72d681d68e Mon Sep 17 00:00:00 2001 From: ScriptedAlchemy Date: Tue, 1 Sep 2026 22:51:03 +0000 Subject: [PATCH 1/2] fix(workbench): repeated-flag usage markers, optional-boolean omission, stale MCP prefill rejection Mark repeatable named options with the generated help's ` ...` suffix, keep optional undefaulted booleans out of the draft until explicitly set, and surface a missing-tool notice instead of attaching prefilled arguments to the first advertised tool. --- .changeset/route-input-usage-prefill-fixes.md | 12 +++++ .../workbench-post-merge-review-fixes.md | 6 +++ packages/workbench/src/mcp/mcp-page.css | 8 ++++ packages/workbench/src/mcp/mcp-page.tsx | 11 ++++- packages/workbench/src/routes/routes-model.ts | 33 +++++++++---- packages/workbench/src/routes/routes-page.tsx | 33 ++++++++++--- .../workbench/tests/examples-real.e2e.test.ts | 4 +- packages/workbench/tests/mcp-page.test.ts | 27 +++++++++++ packages/workbench/tests/routes-model.test.ts | 48 ++++++++++++++++++- packages/workbench/tests/routes-page.test.ts | 4 ++ 10 files changed, 166 insertions(+), 20 deletions(-) create mode 100644 .changeset/route-input-usage-prefill-fixes.md create mode 100644 .changeset/workbench-post-merge-review-fixes.md diff --git a/.changeset/route-input-usage-prefill-fixes.md b/.changeset/route-input-usage-prefill-fixes.md new file mode 100644 index 000000000..7c4cacb26 --- /dev/null +++ b/.changeset/route-input-usage-prefill-fixes.md @@ -0,0 +1,12 @@ +--- +"agent-bundle": patch +--- + +Route catalog and MCP prefill correctness fixes from post-merge review: CLI +usage summaries mark repeatable named options with the same ` ...` operand +suffix the generated help prints; optional booleans without a schema default +keep an unset state (a three-state omitted/true/false control) instead of +submitting an explicit `false` the handler can observe; and a stale +Routes-page prefill naming a tool the server no longer advertises surfaces a +missing-tool notice instead of silently attaching the prepared arguments to +the first advertised tool. diff --git a/.changeset/workbench-post-merge-review-fixes.md b/.changeset/workbench-post-merge-review-fixes.md new file mode 100644 index 000000000..8c3344934 --- /dev/null +++ b/.changeset/workbench-post-merge-review-fixes.md @@ -0,0 +1,6 @@ +--- +"agent-bundle": patch +--- + +Preserve repeated CLI option markers, optional boolean omission, and stale +MCP tool-prefill safety in the Workbench route workflow. diff --git a/packages/workbench/src/mcp/mcp-page.css b/packages/workbench/src/mcp/mcp-page.css index 81370cafc..e8c1b4f0f 100644 --- a/packages/workbench/src/mcp/mcp-page.css +++ b/packages/workbench/src/mcp/mcp-page.css @@ -337,6 +337,14 @@ color: #a8b8ca; } +.mcp-page-missing-tool { + background: #3a1f24; + border-left: 3px solid #f28b82; + color: #ffd9d4; + margin: 0 0 0.75rem; + padding: 0.55rem 0.75rem; +} + @media (max-width: 42rem) { .mcp-page-heading, .mcp-page-catalog li, diff --git a/packages/workbench/src/mcp/mcp-page.tsx b/packages/workbench/src/mcp/mcp-page.tsx index 1dda422dd..fa4fa6c4e 100644 --- a/packages/workbench/src/mcp/mcp-page.tsx +++ b/packages/workbench/src/mcp/mcp-page.tsx @@ -1240,7 +1240,13 @@ export const McpPage = (props: McpPageProps) => { const prompts = catalogItems(model.catalogs.prompts, 'Prompt'); const resources = catalogItems(model.catalogs.resources, 'Resource'); const resourceTemplates = catalogItems(model.catalogs.resourceTemplates, 'Resource template'); - const selectedTool = tools.find((item) => item.name === toolName) ?? tools[0]; + const matchedTool = tools.find((item) => item.name === toolName); + const selectedTool = matchedTool ?? (initialToolPrefill === undefined && toolName === '' ? tools[0] : undefined); + const missingToolName = initialToolPrefill !== undefined + && model.phase === 'ready' + && !tools.some((item) => item.name === initialToolPrefill.toolName) + ? initialToolPrefill.toolName + : undefined; const selectedPrompt = prompts.find((item) => item.name === promptName) ?? prompts[0]; const active = Object.values(model.activeRequests); const controls = mcpPageSessionControls(model.phase, pendingActions, onResetSession !== undefined, serverCatalogState); @@ -1428,6 +1434,9 @@ export const McpPage = (props: McpPageProps) => { Tool call prefilled from Routes

{initialToolPrefill.serverName} ยท {initialToolPrefill.toolName}

{display(initialToolPrefill.arguments)}
+ {missingToolName === undefined ? undefined :

+ The server no longer advertises the "{missingToolName}" tool. The prepared arguments were not applied to another tool. +

}

Open the session and use the existing call control when you are ready. Nothing runs automatically.

}
diff --git a/packages/workbench/src/routes/routes-model.ts b/packages/workbench/src/routes/routes-model.ts index f923611f6..df48a9fd1 100644 --- a/packages/workbench/src/routes/routes-model.ts +++ b/packages/workbench/src/routes/routes-model.ts @@ -213,24 +213,35 @@ export const routeCatalogHasKind = (catalog: RouteCatalog, kind: RouteManifestKi export const routeCatalogServerCount = (catalog: RouteCatalog): number => catalog.servers.length; -const defaultDraftValue = (schema: RouteInputPropertySchema): RouteInputDraftValue => { +/** + * An optional boolean without a schema default stays out of the draft: a + * `false` initialization would submit `{ key: false }` where the author's + * handler observes an omitted property, defeating optional semantics. + */ +const defaultDraftValue = ( + schema: RouteInputPropertySchema, + required: boolean, +): RouteInputDraftValue | undefined => { if (schema.default !== undefined) { if (Array.isArray(schema.default)) { return Object.freeze(schema.default.map((value) => typeof value === 'boolean' ? value : String(value))); } return typeof schema.default === 'boolean' ? schema.default : String(schema.default); } - if (schema.type === 'boolean') return false; + if (schema.type === 'boolean') return required ? false : undefined; if (schema.type === 'array') return Object.freeze([]); return ''; }; -export const createRouteInputDraft = (schema: RouteInputSchema): RouteInputDraft => Object.freeze( - Object.fromEntries(Object.keys(schema.properties).sort().map((key) => [ - key, - defaultDraftValue(schema.properties[key]!), - ])), -); +export const createRouteInputDraft = (schema: RouteInputSchema): RouteInputDraft => { + const required = new Set(schema.required ?? []); + return Object.freeze(Object.fromEntries( + Object.keys(schema.properties).sort().flatMap((key) => { + const value = defaultDraftValue(schema.properties[key]!, required.has(key)); + return value === undefined ? [] : [[key, value] as const]; + }), + )); +}; export const routeInputLabel = (key: string): string => { const words = key @@ -357,7 +368,11 @@ export const cliCommandUsage = (command: RouteManifestCliCommand): string => { : `[${option.option}${option.repeated ? '...' : ''}]`); const flags = command.options.filter((option) => option.positional === undefined) .map((option) => { - const value = option.kind === 'boolean' ? `--${option.option}` : `--${option.option} ${cliOperand(option)}`; + // Booleans are flags and the grammar rejects boolean arrays, so only + // value-carrying options can repeat; ` ...` mirrors cli-entry help rows. + const value = option.kind === 'boolean' + ? `--${option.option}` + : `--${option.option} ${cliOperand(option)}${option.repeated ? ' ...' : ''}`; return option.required ? value : `[${value}]`; }); return [...command.path, ...positionals, ...flags].join(' '); diff --git a/packages/workbench/src/routes/routes-page.tsx b/packages/workbench/src/routes/routes-page.tsx index 148cb8ed6..537416df6 100644 --- a/packages/workbench/src/routes/routes-page.tsx +++ b/packages/workbench/src/routes/routes-page.tsx @@ -77,13 +77,26 @@ const scalarControl = ( routeId: string, key: string, schema: Exclude, + required: boolean, value: RouteInputDraftValue | undefined, - setValue: (value: RouteInputDraftValue) => void, + setValue: (value: RouteInputDraftValue | undefined) => void, ): React.ReactNode => { const id = editorId(routeId, key); switch (schema.type) { case 'boolean': - return setValue(event.currentTarget.checked)} type="checkbox" />; + // A checkbox cannot express "unset", so an optional boolean without a + // schema default keeps a third omitted state instead of submitting false. + return required || schema.default !== undefined + ? setValue(event.currentTarget.checked)} type="checkbox" /> + : ; case 'number': return setValue(event.currentTarget.value)} type="number" value={typeof value === 'string' ? value : ''} />; case 'string': @@ -123,8 +136,14 @@ const RouteInputEditor = ({ entry, group, onOpenMcp }: { ? undefined : cliCommandInvocation(entry.command, validated.arguments)); }; - const setValue = (key: string, value: RouteInputDraftValue): void => { - const next = Object.freeze({ ...draft, [key]: value }); + const setValue = (key: string, value: RouteInputDraftValue | undefined): void => { + const entries = { ...draft }; + if (value === undefined) { + delete entries[key]; + } else { + entries[key] = value; + } + const next = Object.freeze(entries); setDraft(next); commitValidation(next); }; @@ -181,7 +200,7 @@ const RouteInputEditor = ({ entry, group, onOpenMcp }: { if (property.type !== 'array') { return
{property.description === undefined ? undefined :

{property.description}

} {error === undefined ? undefined : {error}} @@ -192,7 +211,9 @@ const RouteInputEditor = ({ entry, group, onOpenMcp }: { {label}{required ? ' (required)' : ''} {property.description === undefined ? undefined :

{property.description}

} {values.map((value, index) =>
- {scalarControl(entry.id, `${key}-${String(index)}`, property.items, value, (nextValue) => { + {/* Array rows exist only after "Add", so items are always set. */} + {scalarControl(entry.id, `${key}-${String(index)}`, property.items, true, value, (nextValue) => { + if (nextValue === undefined) return; const next = [...values]; next[index] = nextValue as boolean | string; setValue(key, Object.freeze(next)); diff --git a/packages/workbench/tests/examples-real.e2e.test.ts b/packages/workbench/tests/examples-real.e2e.test.ts index 9babdb9e8..4c4d6557a 100644 --- a/packages/workbench/tests/examples-real.e2e.test.ts +++ b/packages/workbench/tests/examples-real.e2e.test.ts @@ -606,10 +606,10 @@ e2e('renders the flagship compiled route catalog by server and kind in real Chro await expect(inventoryTool.getByLabel('Source (required)')).toBeVisible({ timeout: browserTimeout }); await expect(inventoryTool.getByLabel('Report')).toBeVisible({ timeout: browserTimeout }); await expect(inventoryTool.getByLabel('Strict')).toBeVisible({ timeout: browserTimeout }); + await expect(inventoryTool.getByLabel('Strict')).toHaveValue('', { timeout: browserTimeout }); await inventoryTool.getByRole('button', { name: 'Validate input' }).click(); await expect(inventoryTool.getByRole('alert')).toHaveText('Source is required.', { timeout: browserTimeout }); await inventoryTool.getByLabel('Source (required)').fill('/tmp/audiobooks'); - await inventoryTool.getByLabel('Strict').check(); await expect(inventoryTool.getByRole('button', { name: 'Open in MCP session' })).toBeEnabled({ timeout: browserTimeout }); await inventoryTool.getByRole('button', { name: 'Open in MCP session' }).click(); await waitForSettledWorkbench(page); @@ -618,7 +618,7 @@ e2e('renders the flagship compiled route catalog by server and kind in real Chro const prefill = page.getByRole('status').filter({ hasText: 'Tool call prefilled from Routes' }); await expect(prefill).toContainText('inventory_sources', { timeout: browserTimeout }); await expect(prefill).toContainText('"source": "/tmp/audiobooks"', { timeout: browserTimeout }); - await expect(prefill).toContainText('"strict": true', { timeout: browserTimeout }); + await expect(prefill).not.toContainText('"strict"', { timeout: browserTimeout }); await expect(page.getByRole('button', { name: 'Open MCP session' })).toBeEnabled({ timeout: browserTimeout }); await page.getByRole('link', { name: 'Routes', exact: true }).click(); await waitForSettledWorkbench(page); diff --git a/packages/workbench/tests/mcp-page.test.ts b/packages/workbench/tests/mcp-page.test.ts index 7ef233724..78bdb8ce7 100644 --- a/packages/workbench/tests/mcp-page.test.ts +++ b/packages/workbench/tests/mcp-page.test.ts @@ -462,6 +462,33 @@ describe('MCP page', () => { expect(markup).toContain('weather'); expect(markup).toContain('Berlin'); expect(markup).toContain('Call weather'); + expect(markup).not.toContain('no longer advertises'); + expect(pageController.history).toHaveLength(1); + }); + + it('rejects a stale Routes-page tool prefill without selecting another tool', () => { + const pageController = controller(); + const readyPageController: McpPageController = { + ...pageController, + model: { ...model, phase: 'ready' } as McpBrowserSessionModel, + }; + const markup = renderToStaticMarkup(createElement(McpPage, { + controller: readyPageController, + epochOptions: ['epoch-1'], + initialBinding: { epochId: 'epoch-1', serverName: 'weather', target: 'codex' }, + initialToolPrefill: { + arguments: { city: 'Berlin' }, + serverName: 'weather', + toolName: 'retired_weather', + }, + serverOptions: [{ name: 'weather', target: 'codex' }], + targetOptions: ['codex'], + })); + + expect(markup).toContain('The server no longer advertises the "retired_weather" tool.'); + expect(markup).not.toContain('id="mcp-tool-arguments"'); + expect(markup).not.toContain('Call weather'); + expect(markup).not.toContain('Call clock'); expect(pageController.history).toHaveLength(1); }); diff --git a/packages/workbench/tests/routes-model.test.ts b/packages/workbench/tests/routes-model.test.ts index a747532fd..2885aa368 100644 --- a/packages/workbench/tests/routes-model.test.ts +++ b/packages/workbench/tests/routes-model.test.ts @@ -1,6 +1,6 @@ import { expect, it } from '@rstest/core'; -import type { RouteManifest } from '../../agent-bundle/src/contracts/routes.ts'; +import type { RouteInputSchema, RouteManifest } from '../../agent-bundle/src/contracts/routes.ts'; import { cliCommandInvocation, cliCommandUsage, @@ -219,6 +219,37 @@ it('prefills projected defaults and validates typed route input before invoke', }); }); +it('preserves optional boolean omission while validating explicit and required values', () => { + const schema: RouteInputSchema = { + additionalProperties: false, + properties: { + defaulted: { default: true, type: 'boolean' }, + enabled: { type: 'boolean' }, + strict: { type: 'boolean' }, + }, + required: ['enabled'], + type: 'object', + }; + const draft = createRouteInputDraft(schema); + + expect(draft).toEqual({ defaulted: true, enabled: false }); + expect(validateRouteInput(schema, draft)).toEqual({ + arguments: { defaulted: true, enabled: false }, + errors: {}, + }); + expect(validateRouteInput(schema, { ...draft, strict: true })).toEqual({ + arguments: { defaulted: true, enabled: false, strict: true }, + errors: {}, + }); + expect(validateRouteInput(schema, { ...draft, strict: false })).toEqual({ + arguments: { defaulted: true, enabled: false, strict: false }, + errors: {}, + }); + expect(validateRouteInput(schema, { defaulted: true })).toEqual({ + errors: { enabled: 'Enabled must be true or false.' }, + }); +}); + it('validates the raw JSON fallback without inventing a schema', () => { expect(validateRawRouteInput('{')).toEqual({ error: 'Enter a valid JSON object.' }); expect(validateRawRouteInput('[]')).toEqual({ error: 'Arguments must be a JSON object.' }); @@ -242,7 +273,7 @@ it('formats CLI usage and a shell-copyable invocation from validated input', () }; expect(cliCommandUsage(command)).toBe( - 'library audit [--format ] [--tag ] [--verbose]', + 'library audit [--format ] [--tag ...] [--verbose]', ); expect(cliCommandInvocation(command, { format: 'json', @@ -252,6 +283,19 @@ it('formats CLI usage and a shell-copyable invocation from validated input', () })).toBe("library audit '/Audio Books' --format json --tag fiction --tag history --verbose"); }); +it('marks required and optional repeated named flags in CLI usage', () => { + expect(cliCommandUsage({ + aliases: [], + exitCode: 'zero', + options: [ + { key: 'source', kind: 'string', option: 'source', repeated: true, required: true }, + { key: 'tag', kind: 'string', option: 'tag', repeated: true, required: false }, + ], + path: ['library', 'import'], + routeId: 'cli:library/import', + })).toBe('library import --source ... [--tag ...]'); +}); + it('maps a tool route server id and final route segment into an MCP prefill', () => { const catalog = routeCatalogFor(manifest); const group = catalog.groups[0]!; diff --git a/packages/workbench/tests/routes-page.test.ts b/packages/workbench/tests/routes-page.test.ts index 3316055f3..a74a77b9e 100644 --- a/packages/workbench/tests/routes-page.test.ts +++ b/packages/workbench/tests/routes-page.test.ts @@ -52,7 +52,9 @@ const manifest: RouteManifest = { additionalProperties: false, properties: { count: { default: 2, description: 'Repeat count.', type: 'number' }, + enabled: { default: true, type: 'boolean' }, format: { enum: ['text', 'json'], type: 'string' }, + strict: { type: 'boolean' }, tags: { items: { type: 'string' }, type: 'array' }, }, required: ['format'], @@ -145,8 +147,10 @@ it('renders generated fields, descriptions, defaults, required markers, and a ga expect(markup).toContain('Generated input editor'); expect(markup).toContain('Repeat count.'); expect(markup).toContain('value="2"'); + expect(markup).toMatch(/Enabled]*type="checkbox"[^>]*checked=""/u); expect(markup).toContain('Format (required)'); expect(markup).toContain(''); + expect(markup).toMatch(/Strict]*>