mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-06 08:01:35 +00:00
fix: keep free-form object args, and refuse a run form before plan mode writes
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
fd57dc00be
commit
8537ff428b
@@ -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 —
|
||||
|
||||
@@ -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')
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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', () => {
|
||||
|
||||
@@ -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 {
|
||||
: `<file: ${(bytes / 1024 / 1024).toFixed(1)} MB>`
|
||||
}
|
||||
|
||||
/**
|
||||
* {@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<string, any>,
|
||||
schema: { properties?: Record<string, any> } | undefined,
|
||||
isLeaf: (prop: any) => boolean,
|
||||
visit: (value: unknown, prop: any, path: string) => unknown
|
||||
): Record<string, any> {
|
||||
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<string, any> } | undefined,
|
||||
strippedKeys?: string[]
|
||||
): Record<string, any> {
|
||||
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<string, any> } | undefined,
|
||||
strippedKeys?: string[]
|
||||
): Record<string, any> {
|
||||
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<string, any>,
|
||||
schema: { properties?: Record<string, any> } | undefined
|
||||
): Record<string, any> {
|
||||
const properties = schema?.properties
|
||||
if (!properties) return args
|
||||
return mapMatchingArgs(args, properties, isSecretProp, (value) =>
|
||||
return mapMatchingOrCopy(args, schema, isSecretProp, (value) =>
|
||||
value == null ? undefined : '<hidden>'
|
||||
)
|
||||
}
|
||||
@@ -341,10 +361,8 @@ export function redactFileArgs(
|
||||
args: Record<string, any>,
|
||||
schema: { properties?: Record<string, any> } | undefined
|
||||
): Record<string, any> {
|
||||
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)
|
||||
)
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user