From 55156962d65f0207c8d9f19ed5fea1e91390ea01 Mon Sep 17 00:00:00 2001 From: AlexRV12 <71396855+AlexRV12@users.noreply.github.com> Date: Tue, 25 Aug 2026 17:26:56 +0200 Subject: [PATCH] fix: drop a non-array in a dyn-multiselect slot before it crashes the run form Co-Authored-By: Claude Opus 5 (1M context) --- .../copilot/chat/AIChatManager.svelte.ts | 2 +- .../copilot/chat/AIChatManager.test.ts | 27 +++--------------- .../components/copilot/chat/global/core.ts | 4 +-- frontend/src/lib/components/job_args.test.ts | 28 +++++++++++++++++++ frontend/src/lib/components/job_args.ts | 16 +++++++++-- 5 files changed, 49 insertions(+), 28 deletions(-) diff --git a/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts b/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts index 76e779a45c..b8664c1221 100644 --- a/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts +++ b/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts @@ -3373,7 +3373,7 @@ export class AIChatManager { ) if (messages.length === this.messages.length) return checkpointedShape = shape - const display = this.settledToolDisplay(this.displayMessages, 'Interrupted') + const display = this.#interruptedSnapshot() // onMessageEnd is what gives streamed text its bubble, and it clears // currentReply doing so — text still there has none, and without one the // reply returns as context the reader cannot see. diff --git a/frontend/src/lib/components/copilot/chat/AIChatManager.test.ts b/frontend/src/lib/components/copilot/chat/AIChatManager.test.ts index 064d9b2764..937afc0677 100644 --- a/frontend/src/lib/components/copilot/chat/AIChatManager.test.ts +++ b/frontend/src/lib/components/copilot/chat/AIChatManager.test.ts @@ -231,28 +231,6 @@ describe('AIChatManager run form', () => { // restored into a fresh manager, which has none of the turn's callbacks. Cancel is // then the card's only exit, and until it settles pendingUserAction keeps the whole // session reading as needs-confirmation. - it('settles a restored form whose callback is gone', async () => { - const manager = new AIChatManager() - manager.displayMessages = [ - { - role: 'tool', - tool_call_id: 'call_r', - content: 'Waiting for you to confirm the arguments of "f/a/b"', - isLoading: true, - runForm: { path: 'f/a/b', schema: {}, args: {} } - } - ] - expect(manager.isRunFormPending('call_r')).toBe(false) - - manager.handleRunFormCancel('call_r') - - const { pendingUserAction } = await import('./shared') - const settled = manager.displayMessages[0] - expect(settled.runForm?.canceled).toBe(true) - expect(settled.isLoading).toBe(false) - expect(pendingUserAction(manager.displayMessages)).toBe(undefined) - }) - // A save that fires mid-turn (jobs tray, review dock) stores a transcript nothing // will resume. Storing a card still pending brings back a form whose Run resolves // no callback. @@ -273,9 +251,12 @@ describe('AIChatManager run form', () => { manager.dismissJob('nope') await Promise.resolve() + const { isActiveRunForm } = await import('./shared') const stored = saveChat.mock.calls.at(-1)?.[0]?.[0] - expect(stored?.isLoading).toBe(false) expect(stored?.runForm?.canceled).toBe(true) + // What every mid-turn save has to hold: no stored card renders a live form. A + // save path added without settling would restore a Run that resolves nothing. + expect(isActiveRunForm(stored!)).toBe(false) // The live card is untouched — the turn is still parked on it. expect(manager.displayMessages[0].isLoading).toBe(true) }) diff --git a/frontend/src/lib/components/copilot/chat/global/core.ts b/frontend/src/lib/components/copilot/chat/global/core.ts index 9cbce981c4..fe3751777b 100644 --- a/frontend/src/lib/components/copilot/chat/global/core.ts +++ b/frontend/src/lib/components/copilot/chat/global/core.ts @@ -4318,8 +4318,8 @@ export const SESSION_PREVIEW_TOOL_NAMES = new Set([ 'list_artifact_versions' ]) -// Withheld from the side-panel chat: the argument form is a blocking card, and -// sessions are the only surface that renders one. +// Withheld from the side-panel chat, which is scoped to authoring what is open in the +// editor rather than running deployed scripts. Both surfaces can render the form. const SESSION_ONLY_TOOL_NAMES = new Set(['run_script']) /** diff --git a/frontend/src/lib/components/job_args.test.ts b/frontend/src/lib/components/job_args.test.ts index 442956726e..b2487ca28a 100644 --- a/frontend/src/lib/components/job_args.test.ts +++ b/frontend/src/lib/components/job_args.test.ts @@ -113,6 +113,22 @@ describe('conformArgsToSchema', () => { dropped: { undeclared: [], unshowable: ['name'] } }) }) + + // MultiSelect maps over the value while rendering, so a non-array here throws and + // takes the whole form with it — the user cannot even cancel what they were shown. + it('drops a non-array in a dyn-multiselect slot', () => { + const schema = { + properties: { tenants: { type: 'object', format: 'dynmultiselect-list_tenants' } } + } + expect(conformArgsToSchema({ tenants: { evil: 1 } }, schema)).toMatchObject({ + args: {}, + dropped: { unshowable: ['tenants'] } + }) + expect(conformArgsToSchema({ tenants: 'acme' }, schema)).toMatchObject({ + args: {}, + dropped: { unshowable: ['tenants'] } + }) + }) }) describe('enforceDisabledDefaults', () => { @@ -154,6 +170,18 @@ describe('enforceDisabledDefaults', () => { expect(resetKeys).toEqual(['top', 'cfg.force', 'list[0].mode', 'either.level']) }) + // By value: a locked object default never matches by identity, so every run of such + // a field reported a reset, and the caller was corrected for getting it right. + it('reports no reset for an object default the caller already matched', () => { + const objSchema = { + properties: { opts: { type: 'object', disabled: true, default: { dry_run: true } } } + } + expect(enforceDisabledDefaults({ opts: { dry_run: true } }, objSchema).resetKeys).toEqual([]) + expect(enforceDisabledDefaults({ opts: { dry_run: false } }, objSchema).resetKeys).toEqual([ + 'opts' + ]) + }) + it('reports only the arguments it actually overwrote', () => { const { args, resetKeys } = enforceDisabledDefaults({ free: 'kept' }, schema) // The default still runs; the caller supplied nothing to overwrite, and one told diff --git a/frontend/src/lib/components/job_args.ts b/frontend/src/lib/components/job_args.ts index a67a5924db..bb7c253cd0 100644 --- a/frontend/src/lib/components/job_args.ts +++ b/frontend/src/lib/components/job_args.ts @@ -20,7 +20,9 @@ export function enforceDisabledDefaults( const result = mapMatchingArgs(args, schema.properties, isLockedProp, (value, prop, path) => { // An argument never supplied was not overwritten: the field shows the default // either way, and a caller told otherwise would try to correct what it never sent. - if (value !== undefined && value !== prop.default) resetKeys.push(path) + // By value, since a default can be an object or an array: identity would report + // every run of such a field as overridden, the caller that got it right included. + if (value !== undefined && !deepEqual(value, prop.default)) resetKeys.push(path) return prop.default }) return { args: result, resetKeys } @@ -93,8 +95,18 @@ const DROP = Symbol('drop') /** Types `setInputCat` routes to a widget bound to a scalar. */ const SCALAR_TYPES = new Set(['string', 'number', 'integer', 'boolean']) +/** + * Declares an array even though its `type` says `object`, and the only slot where a + * mismatch is worse than unreadable: `MultiSelect` maps over the value as it renders, + * so anything else throws and takes the whole form down — Cancel with it. + */ +const declaresDynMultiselect = (prop: any) => + typeof prop?.format === 'string' && prop.format.startsWith('dynmultiselect-') + function dropUndeclaredNested(value: any, prop: any, dropped: DroppedPaths, path: string): any { - if (value == null || typeof value !== 'object') return value + if (value == null) return value + if (declaresDynMultiselect(prop) && !Array.isArray(value)) return DROP + if (typeof value !== 'object') return value // A value the form cannot show is one the user would approve unseen: it matches no // level below, so every filter falls straight through it, and `ArgInput` binds it to // a widget that renders nothing — an object in a list slot, or in a scalar input.