mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-06 08:01:35 +00:00
fix: drop run-form arguments shaped unlike their schema, and name every one removed
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
4bf1432d6d
commit
6f4064aa20
@@ -4594,9 +4594,16 @@ export class AIChatManager {
|
||||
): DisplayMessage[] =>
|
||||
messages.map((message) => {
|
||||
if (message.role === 'tool' && (message.isLoading || message.isQueued)) {
|
||||
// Stopping the turn does not stop the job: once the job is queued the script
|
||||
// is running for real, so the card must not claim it was canceled.
|
||||
const ranAlready = message.runForm?.started === true
|
||||
// Stopping the turn does not stop the job, and between Run and the job's id
|
||||
// there is no way to know whether the server queued one: nothing threads the
|
||||
// abort into that request, so it lands either way. That window says so
|
||||
// rather than picking a side — "canceled" hides a script that ran, "started"
|
||||
// invents one that did not.
|
||||
const runState = message.runForm?.started
|
||||
? 'started'
|
||||
: message.runForm?.submitted
|
||||
? 'starting'
|
||||
: 'idle'
|
||||
return {
|
||||
...message,
|
||||
isLoading: false,
|
||||
@@ -4611,17 +4618,21 @@ export class AIChatManager {
|
||||
content: message.userQuestion
|
||||
? `Asked: ${message.userQuestion.question} — ${messageText}`
|
||||
: message.runForm
|
||||
? ranAlready
|
||||
? runState === 'started'
|
||||
? `Run ${message.runForm.path} — started, stopped tracking before it finished`
|
||||
: `Run ${message.runForm.path} — ${messageText}`
|
||||
: runState === 'starting'
|
||||
? `Run ${message.runForm.path} — ${messageText} while starting, check the runs page for a job`
|
||||
: `Run ${message.runForm.path} — ${messageText}`
|
||||
: messageText,
|
||||
// A started run keeps whatever the job reported: it is not this turn's
|
||||
// error, and the jobs tray is still following it.
|
||||
...(ranAlready ? {} : { error: messageText }),
|
||||
// A run that reached the server keeps whatever the job reported: it is not
|
||||
// this turn's error, and the jobs tray is still following it.
|
||||
...(runState === 'idle' ? { error: messageText } : {}),
|
||||
userQuestion: message.userQuestion
|
||||
? { ...message.userQuestion, canceled: true }
|
||||
: undefined,
|
||||
runForm: message.runForm ? { ...message.runForm, canceled: !ranAlready } : undefined
|
||||
runForm: message.runForm
|
||||
? { ...message.runForm, canceled: runState === 'idle' }
|
||||
: undefined
|
||||
}
|
||||
}
|
||||
return message
|
||||
|
||||
@@ -302,9 +302,9 @@ describe('AIChatManager run form', () => {
|
||||
expect(settled.isLoading).toBe(false)
|
||||
})
|
||||
|
||||
// Run flips `submitted` a round trip before the job exists. Stop in that window
|
||||
// cancelled nothing that ran, so the card must not claim the script was left running.
|
||||
it('marks a submitted run cancelled while its job has not started', () => {
|
||||
// Run flips `submitted` a round trip before the job id arrives, and nothing threads
|
||||
// the stop into that request — so the card claims neither outcome for that window.
|
||||
it('claims neither outcome for a run stopped while its job was starting', () => {
|
||||
const manager = new AIChatManager()
|
||||
manager.displayMessages = [
|
||||
{
|
||||
@@ -318,6 +318,29 @@ describe('AIChatManager run form', () => {
|
||||
|
||||
manager.cancelLoadingTools()
|
||||
|
||||
const settled = manager.displayMessages[0]
|
||||
expect(settled.runForm?.canceled).toBe(false)
|
||||
expect(settled.error).toBe(undefined)
|
||||
expect(settled.content).toBe(
|
||||
'Run f/a/b — Canceled while starting, check the runs page for a job'
|
||||
)
|
||||
})
|
||||
|
||||
// Only a form the user never submitted was cancelled outright.
|
||||
it('marks an unsubmitted run cancelled when the turn is stopped', () => {
|
||||
const manager = new AIChatManager()
|
||||
manager.displayMessages = [
|
||||
{
|
||||
role: 'tool',
|
||||
tool_call_id: 'call_u',
|
||||
content: 'Waiting for you to confirm the arguments of "f/a/b"',
|
||||
isLoading: true,
|
||||
runForm: { path: 'f/a/b', schema: {}, args: {} }
|
||||
}
|
||||
]
|
||||
|
||||
manager.cancelLoadingTools()
|
||||
|
||||
const settled = manager.displayMessages[0]
|
||||
expect(settled.runForm?.canceled).toBe(true)
|
||||
expect(settled.content).toBe('Run f/a/b — Canceled')
|
||||
|
||||
@@ -25,14 +25,17 @@
|
||||
// variables have to resolve there too.
|
||||
const workspace = $derived(aiChatManager.operatingWorkspace)
|
||||
|
||||
const properties = $derived(runForm.schema?.properties ?? {})
|
||||
const hasArgs = $derived(Object.keys(properties).length > 0)
|
||||
|
||||
// Deep copy, not a spread: runForm comes off displayMessages ($state), so its nested
|
||||
// Deep copies, not spreads: runForm comes off displayMessages ($state), so its nested
|
||||
// values are proxies that $state() hands back untouched. SchemaForm edits objects and
|
||||
// arrays in place, so a shallow copy would write every keystroke — a password typed
|
||||
// into a nested field included — straight into the persisted transcript.
|
||||
// into a nested field included — straight into the persisted transcript. The schema
|
||||
// goes the same way: SchemaForm binds it and reorders its properties on mount.
|
||||
let args = $state($state.snapshot(runForm.args ?? {}) as Record<string, any>)
|
||||
let schema = $state($state.snapshot(runForm.schema) as Record<string, any>)
|
||||
|
||||
const properties = $derived(schema?.properties ?? {})
|
||||
const hasArgs = $derived(Object.keys(properties).length > 0)
|
||||
|
||||
let isValid = $state(true)
|
||||
let submitting = $state(false)
|
||||
let cardNode = $state<HTMLDivElement | undefined>()
|
||||
@@ -56,8 +59,8 @@
|
||||
processed = await processSecretArgs(
|
||||
// Last gate before the job: what the card showed is what runs, conformed the
|
||||
// same way the prefill was.
|
||||
conformArgsToSchema(args ?? {}, runForm.schema).args,
|
||||
runForm.schema as any,
|
||||
conformArgsToSchema(args ?? {}, schema).args,
|
||||
schema as any,
|
||||
workspace
|
||||
)
|
||||
} catch (e) {
|
||||
@@ -96,7 +99,7 @@
|
||||
<div class="mt-3">
|
||||
{#if hasArgs}
|
||||
<SchemaForm
|
||||
schema={runForm.schema}
|
||||
bind:schema
|
||||
helperScript={{ source: 'deployed', path: runForm.path, runnable_kind: 'script' }}
|
||||
{workspace}
|
||||
prettifyHeader
|
||||
@@ -119,6 +122,12 @@
|
||||
<span class="font-mono">{runForm.resetKeys.join(', ')}</span>
|
||||
</p>
|
||||
{/if}
|
||||
{#if runForm.strippedKeys?.length}
|
||||
<p class="mt-2 text-2xs text-secondary">
|
||||
A secret or a file, so it opened empty for you to fill in:
|
||||
<span class="font-mono">{runForm.strippedKeys.join(', ')}</span>
|
||||
</p>
|
||||
{/if}
|
||||
</div>
|
||||
|
||||
<!-- Both buttons rest while a submit is in flight: the ephemeral variables exist by
|
||||
|
||||
@@ -4733,6 +4733,9 @@ describe('global AI tools', () => {
|
||||
)
|
||||
|
||||
expect(shown).toEqual({ nested: {}, name: 'ada' })
|
||||
// Named, or an emptied field reads as the user having deleted the value and the
|
||||
// next call proposes the same secret again.
|
||||
expect(result).toContain('token, nested.inner')
|
||||
expect(result).not.toContain('hunter2')
|
||||
expect(result).not.toContain('secret_arg')
|
||||
expect(result).not.toContain('prod_api_key')
|
||||
|
||||
@@ -5469,14 +5469,20 @@ async function runDeployedScript(
|
||||
// A secret the model picked is not consent, whatever it holds: a literal is a value
|
||||
// the user never chose, a reference names something the card cannot show them. Files
|
||||
// go the same way — prefilled bytes are bytes the stored transcript then carries.
|
||||
const proposed = stripFileArgs(stripSecretArgs(conformed.args, schema as any), schema as any)
|
||||
const strippedKeys: string[] = []
|
||||
const proposed = stripFileArgs(
|
||||
stripSecretArgs(conformed.args, schema as any, strippedKeys),
|
||||
schema as any,
|
||||
strippedKeys
|
||||
)
|
||||
const form: RunFormDisplay = {
|
||||
path: args.path,
|
||||
summary: script.summary || undefined,
|
||||
schema,
|
||||
args: proposed,
|
||||
droppedKeys: conformed.droppedKeys.length ? conformed.droppedKeys : undefined,
|
||||
resetKeys: conformed.resetKeys.length ? conformed.resetKeys : undefined
|
||||
resetKeys: conformed.resetKeys.length ? conformed.resetKeys : undefined,
|
||||
strippedKeys: strippedKeys.length ? strippedKeys : undefined
|
||||
}
|
||||
|
||||
toolCallbacks.setToolStatus(toolId, {
|
||||
@@ -5548,6 +5554,11 @@ async function runDeployedScript(
|
||||
const reset = conformed.resetKeys.length
|
||||
? `\nThe deployed schema disables ${conformed.resetKeys.join(', ')}, so the form held ${conformed.resetKeys.length > 1 ? 'their defaults' : 'its default'} rather than the proposed ${conformed.resetKeys.length > 1 ? 'values' : 'value'}. Do not propose ${conformed.resetKeys.length > 1 ? 'them' : 'it'} again.`
|
||||
: ''
|
||||
// Otherwise an emptied field reads as the user having deleted it, and the next call
|
||||
// proposes the same secret again.
|
||||
const stripped = strippedKeys.length
|
||||
? `\n${strippedKeys.join(', ')} ${strippedKeys.length > 1 ? 'are secret or file arguments' : 'is a secret or file argument'}, so the form opened ${strippedKeys.length > 1 ? 'them' : 'it'} empty for the user to fill in. ${strippedKeys.length > 1 ? 'They are' : 'It is'} theirs to provide, not yours: do not propose ${strippedKeys.length > 1 ? 'them' : 'it'} again.`
|
||||
: ''
|
||||
// Redacted: a variable path is enough to run a job on a value the model cannot read,
|
||||
// and one shown a path proposes it back on the next call.
|
||||
const submittedJson = JSON.stringify(
|
||||
@@ -5557,7 +5568,7 @@ async function runDeployedScript(
|
||||
submittedJson.length > MAX_SUBMITTED_ARGS_LENGTH
|
||||
? submittedJson.slice(0, MAX_SUBMITTED_ARGS_LENGTH) + '... (truncated)'
|
||||
: submittedJson
|
||||
return `Ran with arguments: ${shown}${dropped}${reset}\n${outcome}`
|
||||
return `Ran with arguments: ${shown}${dropped}${reset}${stripped}\n${outcome}`
|
||||
}
|
||||
|
||||
async function testRunFlowByPath(
|
||||
|
||||
@@ -568,12 +568,16 @@ export type RunFormDisplay = {
|
||||
/** Proposed arguments a disabled field overrode with its default. Named for the same
|
||||
* reason: the field renders locked, so the value it holds is not the proposed one. */
|
||||
resetKeys?: string[]
|
||||
/** Secret and file arguments emptied out of the proposal. Named so an empty field
|
||||
* reads as the caller's value having been removed, not as the field having none. */
|
||||
strippedKeys?: string[]
|
||||
/** Either one unmounts the form, so set exactly one, and only once the loop has
|
||||
* stopped waiting on this card. */
|
||||
submitted?: boolean
|
||||
canceled?: boolean
|
||||
/** The job exists. Distinct from `submitted`, which flips a round trip earlier: a turn
|
||||
* stopped in between must not record a run that never started. */
|
||||
/** The job exists. Distinct from `submitted`, which flips a round trip earlier — in
|
||||
* between, whether the server queued a job is unknown, so a turn stopped there is
|
||||
* recorded as neither started nor canceled. */
|
||||
started?: boolean
|
||||
}
|
||||
|
||||
|
||||
@@ -70,6 +70,29 @@ describe('conformArgsToSchema', () => {
|
||||
expect(args).toEqual({ either: { kind: 'b', level: 2 } })
|
||||
expect(droppedKeys).toEqual(['either.evil'])
|
||||
})
|
||||
|
||||
// A value shaped unlike its schema matches no level below, so every filter walked
|
||||
// past it and the form rendered nothing over an argument the run still carried.
|
||||
it('drops a value whose shape contradicts the declared one', () => {
|
||||
const schema = {
|
||||
properties: {
|
||||
rows: { type: 'array', items: { properties: { token: { type: 'string' } } } },
|
||||
cfg: { type: 'object', properties: { token: { type: 'string' } } },
|
||||
free: { type: 'object' }
|
||||
}
|
||||
}
|
||||
expect(
|
||||
conformArgsToSchema({ rows: { token: '$var:u/ada/prod' }, cfg: [{ token: 'x' }] }, schema)
|
||||
).toMatchObject({ args: {}, droppedKeys: ['rows', 'cfg'] })
|
||||
// One level down too. A free-form object still passes unread: it declares no
|
||||
// structure for the value to contradict, so its contents were never filtered.
|
||||
expect(
|
||||
conformArgsToSchema({ rows: [{ token: 'a' }, ['sneaky']], free: { anything: 1 } }, schema)
|
||||
).toMatchObject({
|
||||
args: { rows: [{ token: 'a' }], free: { anything: 1 } },
|
||||
droppedKeys: ['rows[1]']
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
describe('enforceDisabledDefaults', () => {
|
||||
|
||||
@@ -6,6 +6,7 @@ const isLockedProp = (prop: any) => !!prop?.disabled && 'default' in prop
|
||||
* A field the schema disables is not the caller's to set: whatever it holds, the run
|
||||
* sends the schema's default. Only a field's own `disabled` counts — `SchemaForm`
|
||||
* propagates a parent's downward, so one locked by inheritance alone keeps its value.
|
||||
* It still renders that value in its locked input, so it is unnamed here, not unseen.
|
||||
* Returns the paths it overwrote so the caller can say so; notifying is the caller's job.
|
||||
*/
|
||||
export function enforceDisabledDefaults(
|
||||
@@ -70,26 +71,44 @@ function dropUndeclaredArgs(
|
||||
else droppedKeys.push(keyPath)
|
||||
continue
|
||||
}
|
||||
kept[key] = dropUndeclaredNested(value, properties[key], droppedKeys, keyPath)
|
||||
const nested = dropUndeclaredNested(value, properties[key], droppedKeys, keyPath)
|
||||
if (nested === DROP) droppedKeys.push(keyPath)
|
||||
else kept[key] = nested
|
||||
}
|
||||
// Spread rather than the accumulator itself: $state.snapshot returns a null-prototype
|
||||
// object by identity, and a form editing it in place would write into the stored copy.
|
||||
return { ...kept }
|
||||
}
|
||||
|
||||
/** Stands in for a value the caller must remove rather than keep. */
|
||||
const DROP = Symbol('drop')
|
||||
|
||||
function dropUndeclaredNested(value: any, prop: any, droppedKeys: string[], path: string): any {
|
||||
if (value == null || typeof value !== 'object') return value
|
||||
if (prop?.properties && !Array.isArray(value)) {
|
||||
// A value whose shape contradicts the declared one matches no level below, so every
|
||||
// filter here falls straight through it — and `ArgInput` has no widget for it either
|
||||
// (an object in a list slot renders as nothing at all), so the form would show an
|
||||
// empty field over an argument the run still carries. Only a schema declaring no
|
||||
// structure at all passes a value through unread.
|
||||
const isArray = Array.isArray(value)
|
||||
const declaresArray = prop?.type === 'array' || prop?.items != null
|
||||
const declaresObject =
|
||||
prop?.type === 'object' || prop?.properties != null || Array.isArray(prop?.oneOf)
|
||||
if (isArray ? declaresObject && !declaresArray : declaresArray && !declaresObject) return DROP
|
||||
if (prop?.properties && !isArray) {
|
||||
return dropUndeclaredArgs(value, prop.properties, droppedKeys, path)
|
||||
}
|
||||
if (prop?.items?.properties && Array.isArray(value)) {
|
||||
return value.map((item: any, i: number) =>
|
||||
item != null && typeof item === 'object' && !Array.isArray(item)
|
||||
? dropUndeclaredArgs(item, prop.items.properties, droppedKeys, `${path}[${i}]`)
|
||||
: item
|
||||
)
|
||||
if (prop?.items?.properties && isArray) {
|
||||
const kept: any[] = []
|
||||
value.forEach((item: any, i: number) => {
|
||||
const itemPath = `${path}[${i}]`
|
||||
const nested = dropUndeclaredNested(item, prop.items, droppedKeys, itemPath)
|
||||
if (nested === DROP) droppedKeys.push(itemPath)
|
||||
else kept.push(nested)
|
||||
})
|
||||
return kept
|
||||
}
|
||||
if (Array.isArray(prop?.oneOf) && !Array.isArray(value)) {
|
||||
if (Array.isArray(prop?.oneOf) && !isArray) {
|
||||
// The union of every branch, not the one the tag names: which branch is selected is
|
||||
// runtime state, and pruning by a stale tag would delete what the user typed.
|
||||
const union: Record<string, any> = {}
|
||||
@@ -117,7 +136,10 @@ function mapMatchingArgs(
|
||||
inOneOf = false
|
||||
): any {
|
||||
if (holder == null || typeof holder !== 'object' || Array.isArray(holder)) return holder
|
||||
const result = { ...holder }
|
||||
// Null prototype for the same reason as dropUndeclaredArgs: writing a declared
|
||||
// `__proto__` into a plain `{}` reaches the inherited setter, so the default a
|
||||
// disabled field must enforce would vanish instead of overwriting.
|
||||
const result: Record<string, any> = Object.assign(Object.create(null), holder)
|
||||
for (const [key, prop] of Object.entries<any>(properties)) {
|
||||
const keyPath = path ? `${path}.${key}` : key
|
||||
// A matching object is a leaf, not a level: a password object is stored whole as a
|
||||
@@ -158,7 +180,8 @@ function mapMatchingArgs(
|
||||
}
|
||||
}
|
||||
}
|
||||
return result
|
||||
// Spread rather than the accumulator itself, so callers get a plain object back.
|
||||
return { ...result }
|
||||
}
|
||||
|
||||
const isSecretProp = (prop: any) => !!prop?.password
|
||||
@@ -175,29 +198,40 @@ function fileMarker(base64: string): string {
|
||||
|
||||
/**
|
||||
* 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.
|
||||
* behalf: password fields open empty and the user fills them in. Appends the path of
|
||||
* each one removed, so the caller can be told the field was emptied rather than left to
|
||||
* read the absence as the user having deleted it.
|
||||
*/
|
||||
export function stripSecretArgs(
|
||||
args: Record<string, any>,
|
||||
schema: { properties?: Record<string, any> } | undefined
|
||||
schema: { properties?: Record<string, any> } | undefined,
|
||||
strippedKeys?: string[]
|
||||
): Record<string, any> {
|
||||
const properties = schema?.properties
|
||||
if (!properties) return args
|
||||
return mapMatchingArgs(args, properties, isSecretProp, () => undefined)
|
||||
return mapMatchingArgs(args, properties, isSecretProp, (value, _prop, path) => {
|
||||
if (value !== undefined) strippedKeys?.push(path)
|
||||
return undefined
|
||||
})
|
||||
}
|
||||
|
||||
/**
|
||||
* Drop every file argument, so a caller cannot propose file bytes on the user's behalf:
|
||||
* the field opens empty and the user attaches the file. Bytes a form is prefilled with
|
||||
* are bytes the stored transcript carries, unbounded, for a value no caller can produce.
|
||||
* Reports what it removed for the same reason {@link stripSecretArgs} does.
|
||||
*/
|
||||
export function stripFileArgs(
|
||||
args: Record<string, any>,
|
||||
schema: { properties?: Record<string, any> } | undefined
|
||||
schema: { properties?: Record<string, any> } | undefined,
|
||||
strippedKeys?: string[]
|
||||
): Record<string, any> {
|
||||
const properties = schema?.properties
|
||||
if (!properties) return args
|
||||
return mapMatchingArgs(args, properties, isFileProp, () => undefined)
|
||||
return mapMatchingArgs(args, properties, isFileProp, (value, _prop, path) => {
|
||||
if (value !== undefined) strippedKeys?.push(path)
|
||||
return undefined
|
||||
})
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
Reference in New Issue
Block a user