From 8537ff428bb7ee01ebd1c76cc83a8974e5f70b6f Mon Sep 17 00:00:00 2001 From: AlexRV12 <71396855+AlexRV12@users.noreply.github.com> Date: Wed, 26 Aug 2026 09:10:19 +0200 Subject: [PATCH] fix: keep free-form object args, and refuse a run form before plan mode writes Co-Authored-By: Claude Opus 5 (1M context) --- .../copilot/chat/RunArgsFormDisplay.svelte | 8 ++++ .../copilot/chat/global/core.test.ts | 4 +- .../copilot/chat/planModeMessages.ts | 3 ++ frontend/src/lib/components/job_args.test.ts | 34 ++++++++++++++ frontend/src/lib/components/job_args.ts | 46 +++++++++++++------ 5 files changed, 79 insertions(+), 16 deletions(-) diff --git a/frontend/src/lib/components/copilot/chat/RunArgsFormDisplay.svelte b/frontend/src/lib/components/copilot/chat/RunArgsFormDisplay.svelte index 8b4500c879..edac4033b8 100644 --- a/frontend/src/lib/components/copilot/chat/RunArgsFormDisplay.svelte +++ b/frontend/src/lib/components/copilot/chat/RunArgsFormDisplay.svelte @@ -7,6 +7,7 @@ import { conformArgsToSchema } from '$lib/components/job_args' import { sendUserToast } from '$lib/utils' import { getAiChatManager } from './aiChatManagerContext' + import { PLAN_MODE_MESSAGES } from './planModeMessages' import type { RunFormDisplay } from './shared' // Never the imported singleton: submitting has to resolve the pending callback of @@ -53,6 +54,13 @@ sendUserToast('This run form is no longer active — ask again to run the script.', true) return } + // Ahead of processSecretArgs, which writes ephemeral variables to the workspace: the + // autonomy picker moves while a form sits pending, and the re-gate on the other side + // of the callback runs too late to unmake a write plan mode promised not to do. + if (aiChatManager.planModeActive) { + sendUserToast(PLAN_MODE_MESSAGES.runFormRefused, true) + return + } submitting = true // Last gate before the job: what the card showed is what runs, conformed the same // way the prefill was. Named for the same reason the prefill names what it drops — diff --git a/frontend/src/lib/components/copilot/chat/global/core.test.ts b/frontend/src/lib/components/copilot/chat/global/core.test.ts index 5fd55e1481..826f81a79b 100644 --- a/frontend/src/lib/components/copilot/chat/global/core.test.ts +++ b/frontend/src/lib/components/copilot/chat/global/core.test.ts @@ -5565,8 +5565,8 @@ describe('session-only preview tools gating', () => { expect(names).not.toContain('list_app_runs') expect(names).not.toContain('search_dom') expect(names).not.toContain('read_dom') - // Withheld for its own reason: the side-panel chat cannot render the argument - // form the tool blocks on. + // Withheld for its own reason: scope, not capability — the side-panel chat authors + // what is open in the editor rather than running deployed scripts. expect(names).not.toContain('run_script') // other tools are still present expect(names).toContain('write_script') diff --git a/frontend/src/lib/components/copilot/chat/planModeMessages.ts b/frontend/src/lib/components/copilot/chat/planModeMessages.ts index 0b502ba839..85710d0c54 100644 --- a/frontend/src/lib/components/copilot/chat/planModeMessages.ts +++ b/frontend/src/lib/components/copilot/chat/planModeMessages.ts @@ -12,6 +12,9 @@ export const PLAN_MODE_MESSAGES = { /** Sits beside the autonomy picker while plan mode holds. The picker's tooltip carries * the rest, so this states only the constraint. */ modeNote: 'Read-only', + /** Refuses a pending run form. Its own string because nothing is settled: the form stays + * live, so this names the way out rather than telling the user their run was blocked. */ + runFormRefused: 'Plan mode is read-only — switch it off to run this script.', // One pair for both artifact tools: the fact and the way forward are the same whether the // model tried to mint the plan or to rewrite it, and the generic refusal above ("put this // change in your plan") reads as nonsense for a call that writes a document. diff --git a/frontend/src/lib/components/job_args.test.ts b/frontend/src/lib/components/job_args.test.ts index 9b85fca1a5..bb6d50f370 100644 --- a/frontend/src/lib/components/job_args.test.ts +++ b/frontend/src/lib/components/job_args.test.ts @@ -138,6 +138,32 @@ describe('conformArgsToSchema', () => { }) }) + // What the parsers emit for a bare `dict`/`object` annotation, and `ArgInput` gives it a + // JSON editor: reading the empty declaration set as structure reported every key the + // user typed as one the script has no field for, then ran it with an empty object. + it('leaves a free-form object declared with empty properties alone', () => { + const freeForm = { type: 'object', properties: {} } + expect( + conformArgsToSchema({ cfg: { env: 'prod', retries: 2 } }, { properties: { cfg: freeForm } }) + ).toEqual({ + args: { cfg: { env: 'prod', retries: 2 } }, + resetKeys: [], + dropped: { undeclared: [], unshowable: [] } + }) + // Nested and per element, since the same declaration reaches both. + expect( + conformArgsToSchema( + { outer: { cfg: { env: 'prod' } }, rows: [{ a: 1 }] }, + { + properties: { + outer: { type: 'object', properties: { cfg: freeForm } }, + rows: { type: 'array', items: freeForm } + } + } + ).args + ).toEqual({ outer: { cfg: { env: 'prod' } }, rows: [{ a: 1 }] }) + }) + // The guard reads declared structure, never the declared `type`: a dyn-multiselect is // `type: 'object'` holding an array, and reading `type` dropped what the user picked. it('keeps a dyn-multiselect array and drops an object in a scalar slot', () => { @@ -333,6 +359,14 @@ describe('secret args at every level the form nests', () => { it('leaves no key behind for a level the args never carried', () => { expect(Object.keys(stripSecretArgs({ top: 'x' }, schema))).toEqual([]) }) + + // The caller binds the result to a form that edits in place, so a schema declaring + // nothing must not hand back the object it was given. + it('copies even when the schema declares nothing to strip', () => { + const args = { top: 'x' } + expect(stripSecretArgs(args, undefined)).not.toBe(args) + expect(stripSecretArgs(args, undefined)).toEqual(args) + }) }) describe('redactFileArgs', () => { diff --git a/frontend/src/lib/components/job_args.ts b/frontend/src/lib/components/job_args.ts index 5b4b6c3702..d0e2b3c0fa 100644 --- a/frontend/src/lib/components/job_args.ts +++ b/frontend/src/lib/components/job_args.ts @@ -109,6 +109,15 @@ const SCALAR_TYPES = new Set(['string', 'number', 'integer', 'boolean']) const declaresDynMultiselect = (prop: any) => typeof prop?.format === 'string' && prop.format.startsWith('dynmultiselect-') +/** + * Declares a structure to filter against, rather than a free-form object. An empty + * `properties` is what the parsers emit for a bare `dict`/`object` annotation, and + * `ArgInput` gives it a JSON editor holding whatever the user types — so reading it as + * structure would report every key the editor accepts as an argument nobody declared. + */ +const declaresProperties = (prop: any) => + prop?.properties != null && Object.keys(prop.properties).length > 0 + /** * Whether `prop` declares a slot the form can show `value` in. A value that fits nowhere * is one the user would approve unseen: it matches no level below, so every filter falls @@ -124,7 +133,7 @@ function fitsDeclaredShape(value: any, prop: any): boolean { // Declared nested structure, never the declared `type`: a dyn-multiselect argument is // `type: 'object'` holding an array, so reading `type` would drop what the user picked. const declaresArray = prop?.items != null - const declaresObject = prop?.properties != null || Array.isArray(prop?.oneOf) + const declaresObject = declaresProperties(prop) || Array.isArray(prop?.oneOf) return !(isArray ? declaresObject && !declaresArray : declaresArray && !declaresObject) } @@ -133,7 +142,7 @@ function dropUndeclaredNested(value: any, prop: any, dropped: DroppedPaths, path if (!fitsDeclaredShape(value, prop)) return DROP if (typeof value !== 'object') return value const isArray = Array.isArray(value) - if (prop?.properties && !isArray) { + if (declaresProperties(prop) && !isArray) { return dropUndeclaredArgs(value, prop.properties, dropped, path) } // Every declared element shape, not just an object one: the guards above are what drop @@ -279,6 +288,23 @@ function fileMarker(base64: string): string { : `` } +/** + * {@link mapMatchingArgs} against a schema that may declare nothing to match. Copied even + * then, for the reason {@link enforceDisabledDefaults} copies: callers hand the result to + * a form that edits it in place, and one branch returning the input would write every + * keystroke through to their own copy. + */ +function mapMatchingOrCopy( + args: Record, + schema: { properties?: Record } | undefined, + isLeaf: (prop: any) => boolean, + visit: (value: unknown, prop: any, path: string) => unknown +): Record { + const properties = schema?.properties + if (!properties) return { ...args } + return mapMatchingArgs(args, properties, isLeaf, visit) +} + /** * Drop every password-typed argument, so a caller cannot propose a secret on the user's * behalf: password fields open empty and the user fills them in. Appends the path of @@ -290,9 +316,7 @@ export function stripSecretArgs( schema: { properties?: Record } | undefined, strippedKeys?: string[] ): Record { - const properties = schema?.properties - if (!properties) return args - return mapMatchingArgs(args, properties, isSecretProp, (value, _prop, path) => { + return mapMatchingOrCopy(args, schema, isSecretProp, (value, _prop, path) => { if (value !== undefined) strippedKeys?.push(path) return undefined }) @@ -309,9 +333,7 @@ export function stripFileArgs( schema: { properties?: Record } | undefined, strippedKeys?: string[] ): Record { - const properties = schema?.properties - if (!properties) return args - return mapMatchingArgs(args, properties, isFileProp, (value, _prop, path) => { + return mapMatchingOrCopy(args, schema, isFileProp, (value, _prop, path) => { if (value !== undefined) strippedKeys?.push(path) return undefined }) @@ -325,9 +347,7 @@ export function redactSecretArgs( args: Record, schema: { properties?: Record } | undefined ): Record { - const properties = schema?.properties - if (!properties) return args - return mapMatchingArgs(args, properties, isSecretProp, (value) => + return mapMatchingOrCopy(args, schema, isSecretProp, (value) => value == null ? undefined : '' ) } @@ -341,10 +361,8 @@ export function redactFileArgs( args: Record, schema: { properties?: Record } | undefined ): Record { - const properties = schema?.properties - if (!properties) return args const mark = (value: unknown) => (typeof value === 'string' ? fileMarker(value) : value) - return mapMatchingArgs(args, properties, isFileProp, (value) => + return mapMatchingOrCopy(args, schema, isFileProp, (value) => Array.isArray(value) ? value.map(mark) : mark(value) ) }