mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-10-16 00:03:02 +00:00
fix: refuse a run in the form, and show an adopted job's own arguments
Two findings on the deployed page carrying a chat call. `RunForm.run()` mints a variable per `password` argument before it calls `runAction`, so the plan-mode gate in the detail pages ran after the write it was there to prevent — and the keybinding reached it without the button. The refusal moves into the form, as `runRefusal`: set, it takes no writes at all, its submit buttons are disabled, and `run()` says so and returns before the secret processing. The page also stops offering the JSON editor under plan mode, instead of leaving one mounted that silently discards what is typed. An adopted job's arguments are the page's, and the card had no way to show them: it kept rendering the proposal, so a reader who edited a field saw the wrong values and the model was told nothing. `adopt` now hands them over with the job id, and they go through the same redaction and size guard as a run this tool starts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
88683188ce
commit
27cb8e7ae6
@@ -75,6 +75,13 @@
|
||||
}
|
||||
|
||||
export async function run(overrideScheduledForStr?: string | undefined | null) {
|
||||
// Ahead of everything, including the flush below: the secret processing further down
|
||||
// creates a variable per `password` argument, so a refusal that waits for `runAction`
|
||||
// has already written. The keybinding reaches here without the button.
|
||||
if (runRefusal) {
|
||||
sendUserToast(runRefusal, true)
|
||||
return
|
||||
}
|
||||
// An editor whose text does not parse never wrote it to `args`, so running now would send
|
||||
// the last value that did parse. Flush first: a keystroke still inside the editor debounce
|
||||
// has not been parsed yet, and per-field editors parse it in an effect, hence the tick.
|
||||
@@ -127,11 +134,12 @@
|
||||
extra_perms?: Record<string, boolean>
|
||||
}
|
||||
| undefined
|
||||
/** Take no writes from the reader: no edits, no dynamic-select helper, no JSON. All
|
||||
* three write before Run is pressed — a `password` mints as it is typed, the helper
|
||||
* runs its entrypoint on mount — so gating Run cannot stop them. For a form standing
|
||||
* in for a chat card under plan mode, which promised none of it. */
|
||||
argsReadonly?: boolean
|
||||
/** Set to refuse every write this form can make, and to say so when a run is
|
||||
* attempted. No edits, no dynamic-select helper, no JSON, and no run — `run()` mints
|
||||
* the `password` arguments it is about to send, so refusing in `runAction` is already
|
||||
* too late. For a form standing in for a chat card under plan mode, which promised
|
||||
* none of it. */
|
||||
runRefusal?: string
|
||||
runAction: (
|
||||
scheduledForStr: string | undefined,
|
||||
args: Record<string, any>,
|
||||
@@ -165,7 +173,7 @@
|
||||
let {
|
||||
runnable,
|
||||
runAction,
|
||||
argsReadonly = false,
|
||||
runRefusal = undefined,
|
||||
buttonText = 'Run',
|
||||
schedulable = true,
|
||||
detailed = true,
|
||||
@@ -184,6 +192,8 @@
|
||||
actions = undefined
|
||||
}: Props = $props()
|
||||
|
||||
const argsReadonly = $derived(!!runRefusal)
|
||||
|
||||
let showPsCommonParams = $derived(
|
||||
runnable?.language === 'powershell' && runnable?.schema?.['x-windmill-ps-cmd-binding'] === true
|
||||
)
|
||||
@@ -393,7 +403,7 @@
|
||||
variant="accent"
|
||||
unifiedSize="md"
|
||||
btnClasses="!inline-flex"
|
||||
disabled={!isValid && !jsonView}
|
||||
disabled={(!isValid && !jsonView) || argsReadonly}
|
||||
on:click={() => run()}
|
||||
shortCut={{ Icon: CornerDownLeft, hide: !viewKeybinding }}
|
||||
>
|
||||
@@ -442,7 +452,7 @@
|
||||
variant="accent"
|
||||
unifiedSize="md"
|
||||
btnClasses="!inline-flex"
|
||||
disabled={!isValid && !jsonView}
|
||||
disabled={(!isValid && !jsonView) || argsReadonly}
|
||||
on:click={() => run(null)}
|
||||
shortCut={{ Icon: CornerDownLeft, hide: !viewKeybinding }}
|
||||
>
|
||||
@@ -455,7 +465,7 @@
|
||||
<Button
|
||||
btnClasses="!px-6 !py-1 w-full"
|
||||
variant="accent"
|
||||
disabled={!isValid && !jsonView}
|
||||
disabled={(!isValid && !jsonView) || argsReadonly}
|
||||
on:click={() => run(null)}
|
||||
shortCut={{ Icon: CornerDownLeft, hide: !viewKeybinding }}
|
||||
>
|
||||
|
||||
@@ -2137,9 +2137,15 @@ export class AIChatManager implements ChatViewHost {
|
||||
|
||||
/** The reader ran the item on its own page; the tool follows that job rather than
|
||||
* starting one. False when the call is no longer waiting. */
|
||||
handleRunFormJob = (toolId: string, jobId: string): boolean => {
|
||||
handleRunFormJob = (toolId: string, jobId: string, args: Record<string, any>): boolean => {
|
||||
if (!this.isRunFormPending(toolId)) return false
|
||||
this.#settleRunForm(toolId, { kind: 'job', jobId })
|
||||
// Snapshotted for the same reason the draft is: these are `$state` proxies off the
|
||||
// page's own form, which keeps editing them after the run.
|
||||
this.#settleRunForm(toolId, {
|
||||
kind: 'job',
|
||||
jobId,
|
||||
args: ($state.snapshot(args) ?? {}) as Record<string, any>
|
||||
})
|
||||
return true
|
||||
}
|
||||
|
||||
@@ -2172,7 +2178,7 @@ export class AIChatManager implements ChatViewHost {
|
||||
answer: RunFormAnswer | undefined,
|
||||
card?: (runForm: RunFormDisplay) => Partial<ToolDisplayMessage>
|
||||
) => {
|
||||
const submitted = answer?.kind === 'args' ? answer.args : answer ? {} : undefined
|
||||
const submitted = answer?.args
|
||||
const entry = this.#runForms.get(toolId)
|
||||
// Its draft holds whatever was typed into the form, a minted password included.
|
||||
this.#runForms.delete(toolId)
|
||||
|
||||
@@ -5241,7 +5241,10 @@ describe('global AI tools', () => {
|
||||
vi.mocked(ScriptService.getScriptByPath).mockResolvedValueOnce(script)
|
||||
|
||||
const statuses: any[] = []
|
||||
const requestRunArgs = vi.fn(async (_toolId: string, form: any) => ({ kind: 'args', args: form.args }))
|
||||
const requestRunArgs = vi.fn(async (_toolId: string, form: any) => ({
|
||||
kind: 'args',
|
||||
args: form.args
|
||||
}))
|
||||
const yolo = {
|
||||
...toolCallbacks,
|
||||
setToolStatus: (_toolId: string, status: any) => statuses.push(status),
|
||||
@@ -5438,6 +5441,39 @@ describe('global AI tools', () => {
|
||||
expect(statuses.find((s) => s.runForm)?.runForm?.conversational).toBe(true)
|
||||
})
|
||||
|
||||
// A run confirmed on the item's own page answers with the job it started, so this tool
|
||||
// never sees a submission: the card and the model would otherwise be left describing the
|
||||
// proposal as what ran, which is wrong as soon as the reader edits a field.
|
||||
it('run_script reports an adopted job with the arguments that page sent', async () => {
|
||||
vi.mocked(ScriptService.getScriptByPath).mockResolvedValue({
|
||||
path: 'f/scripts/adopted',
|
||||
schema: { properties: { name: { type: 'string' } } }
|
||||
} as any)
|
||||
|
||||
const statuses: any[] = []
|
||||
const result = await withCompletedTestJob(() =>
|
||||
callGlobalTool(
|
||||
'run_script',
|
||||
{ path: 'f/scripts/adopted', args: { name: 'Ada' } },
|
||||
{
|
||||
...toolCallbacks,
|
||||
setToolStatus: (_id: string, s: any) => statuses.push(s),
|
||||
requestRunArgs: async () => ({
|
||||
kind: 'job',
|
||||
jobId: 'job-123',
|
||||
args: { name: 'Grace' }
|
||||
})
|
||||
}
|
||||
)
|
||||
)
|
||||
|
||||
// Started by the page, so this tool must not start one of its own.
|
||||
expect(JobService.runScriptByPath).not.toHaveBeenCalled()
|
||||
expect(statuses.filter((s) => s.parameters).at(-1)?.parameters).toEqual({ name: 'Grace' })
|
||||
expect(result).toContain('Grace')
|
||||
expect(result).not.toContain('Ada')
|
||||
})
|
||||
|
||||
// The posture answers wherever it is set, form or no form: what it answers is consent, and a
|
||||
// host without one has nothing left to ask. A secret still becomes a reference first, which
|
||||
// is the only thing the missing form would have done.
|
||||
@@ -5551,7 +5587,10 @@ describe('global AI tools', () => {
|
||||
callGlobalTool(
|
||||
'test_run_script',
|
||||
{ path: 'f/scripts/noargs-test', args: { force_delete: true } },
|
||||
{ ...toolCallbacks, requestRunArgs: async (_toolId, form) => ({ kind: 'args', args: form.args }) }
|
||||
{
|
||||
...toolCallbacks,
|
||||
requestRunArgs: async (_toolId, form) => ({ kind: 'args', args: form.args })
|
||||
}
|
||||
)
|
||||
)
|
||||
|
||||
@@ -5576,7 +5615,10 @@ describe('global AI tools', () => {
|
||||
callGlobalTool(
|
||||
'run_script',
|
||||
{ path: 'f/scripts/host-literal', args: { token: 'hunter2' } },
|
||||
{ ...toolCallbacks, requestRunArgs: async (_toolId, form) => ({ kind: 'args', args: form.args }) }
|
||||
{
|
||||
...toolCallbacks,
|
||||
requestRunArgs: async (_toolId, form) => ({ kind: 'args', args: form.args })
|
||||
}
|
||||
)
|
||||
)
|
||||
|
||||
@@ -5703,7 +5745,10 @@ describe('global AI tools', () => {
|
||||
path: 'u/admin/live_flow',
|
||||
args: { name: 'Ada' }
|
||||
},
|
||||
{ ...toolCallbacks, requestRunArgs: async () => ({ kind: 'args', args: { name: 'Grace' } }) },
|
||||
{
|
||||
...toolCallbacks,
|
||||
requestRunArgs: async () => ({ kind: 'args', args: { name: 'Grace' } })
|
||||
},
|
||||
{ testActiveFlow }
|
||||
)
|
||||
)
|
||||
@@ -6544,7 +6589,10 @@ describe('global AI tools', () => {
|
||||
callGlobalTool(
|
||||
'test_run_step',
|
||||
{ path: 'f/flows/drifted-step', stepId: 'call_script', args: { name: 'Ada' } },
|
||||
{ ...toolCallbacks, requestRunArgs: async (_toolId, f) => ((form = f), { kind: 'args', args: f.args }) }
|
||||
{
|
||||
...toolCallbacks,
|
||||
requestRunArgs: async (_toolId, f) => ((form = f), { kind: 'args', args: f.args })
|
||||
}
|
||||
)
|
||||
)
|
||||
|
||||
@@ -6583,7 +6631,10 @@ describe('global AI tools', () => {
|
||||
callGlobalTool(
|
||||
'test_run_step',
|
||||
{ path: 'f/flows/secretful-step', stepId: 'call_secretful', args: {} },
|
||||
{ ...toolCallbacks, requestRunArgs: async (_toolId, f) => ((form = f), { kind: 'args', args: f.args }) }
|
||||
{
|
||||
...toolCallbacks,
|
||||
requestRunArgs: async (_toolId, f) => ((form = f), { kind: 'args', args: f.args })
|
||||
}
|
||||
)
|
||||
)
|
||||
|
||||
@@ -6612,7 +6663,10 @@ describe('global AI tools', () => {
|
||||
callGlobalTool(
|
||||
'test_run_step',
|
||||
{ path: 'f/flows/preprocessed', stepId: 'preprocessor', args: { event: 'signup' } },
|
||||
{ ...toolCallbacks, requestRunArgs: async (_toolId, f) => ((form = f), { kind: 'args', args: f.args }) }
|
||||
{
|
||||
...toolCallbacks,
|
||||
requestRunArgs: async (_toolId, f) => ((form = f), { kind: 'args', args: f.args })
|
||||
}
|
||||
)
|
||||
)
|
||||
|
||||
@@ -6812,7 +6866,10 @@ describe('global AI tools', () => {
|
||||
callGlobalTool(
|
||||
'run_script',
|
||||
{ path: 'f/scripts/greet', args: { name: 'Ada' } },
|
||||
{ ...toolCallbacks, requestRunArgs: async () => ({ kind: 'args', args: { name: 'Grace' } }) }
|
||||
{
|
||||
...toolCallbacks,
|
||||
requestRunArgs: async () => ({ kind: 'args', args: { name: 'Grace' } })
|
||||
}
|
||||
)
|
||||
)
|
||||
|
||||
@@ -6839,7 +6896,10 @@ describe('global AI tools', () => {
|
||||
callGlobalTool(
|
||||
'run_script',
|
||||
{ path: 'f/scripts/greet', args: { name: 'Ada' } },
|
||||
{ ...toolCallbacks, requestRunArgs: async (_toolId, form) => ({ kind: 'args', args: form.args }) }
|
||||
{
|
||||
...toolCallbacks,
|
||||
requestRunArgs: async (_toolId, form) => ({ kind: 'args', args: form.args })
|
||||
}
|
||||
)
|
||||
)
|
||||
|
||||
|
||||
@@ -6363,11 +6363,11 @@ async function runThroughForm(spec: FormRunSpec, ctx: WriteDraftCtx): Promise<st
|
||||
return runFormCancelled(spec.toolName, noun)
|
||||
}
|
||||
|
||||
// A run the reader started on the item's own page. It is already going, with that page's
|
||||
// own arguments and version, so there is nothing here to gate, mint or redact — only a job
|
||||
// to follow, which is what makes the chat able to report on it.
|
||||
const adopted = answer.kind === 'job' ? answer.jobId : undefined
|
||||
const submitted = answer.kind === 'args' ? answer.args : {}
|
||||
// A run the reader started on the item's own page. It is already going, at that page's own
|
||||
// version and with arguments its own form coerced and minted — so there is nothing here to
|
||||
// gate or process, only a job to follow and its arguments to show.
|
||||
const adopted = answer.kind === 'job' ? answer : undefined
|
||||
const submitted = answer.args
|
||||
|
||||
const blockedBeforeRun = adopted ? undefined : blockedByPlanMode()
|
||||
if (blockedBeforeRun) {
|
||||
@@ -6403,16 +6403,18 @@ async function runThroughForm(spec: FormRunSpec, ctx: WriteDraftCtx): Promise<st
|
||||
// job instead. Only once there is a job to read them from: substituting the marker any
|
||||
// earlier would leave a run that never started showing nothing but the marker.
|
||||
const oversized = JSON.stringify(forCard).length > MAX_PERSISTED_ARGS_LENGTH
|
||||
// An adopted job's arguments are the page's, not these: the card reads them off the job.
|
||||
if (!oversized && !adopted) {
|
||||
if (!oversized) {
|
||||
toolCallbacks.setToolStatus(toolId, { parameters: forCard })
|
||||
} else if (adopted) {
|
||||
// Its job exists already, so the marker can stand in now rather than in `jobStarter`.
|
||||
toolCallbacks.setToolStatus(toolId, { parameters: { reason: 'WINDMILL_TOO_BIG' } })
|
||||
}
|
||||
|
||||
const outcome = await executeTestRun({
|
||||
jobStarter: async () => {
|
||||
if (adopted) {
|
||||
toolCallbacks.markRunFormStarted?.(toolId)
|
||||
return adopted
|
||||
return adopted.jobId
|
||||
}
|
||||
let jobId: string
|
||||
try {
|
||||
@@ -6473,15 +6475,14 @@ async function runThroughForm(spec: FormRunSpec, ctx: WriteDraftCtx): Promise<st
|
||||
// Naming them costs a copy of arguments already in the call above, and cleared/reset/
|
||||
// stripped name every way the form's own differ from the proposed ones — so only what
|
||||
// the user changed is news.
|
||||
// An adopted job is the page's own: its arguments and version are whatever that page held,
|
||||
// which is not readable from here. Saying so beats naming arguments that may not be the
|
||||
// ones that ran — and the notes below all describe the proposal this run did not use.
|
||||
if (adopted) {
|
||||
return `The user ran "${spec.path}" themselves on its own page, with the arguments and version that page held — not necessarily the ones proposed here. Read the job if you need them.\n${outcome}`
|
||||
}
|
||||
const ran = deepEqual(redacted, proposed)
|
||||
? 'Ran with the arguments the form opened with, unedited.'
|
||||
: `Ran with arguments: ${shown}`
|
||||
// The notes below say what THIS form did to the proposal, and an adopted run went through
|
||||
// the page's form instead — so they describe a submission that never happened.
|
||||
if (adopted) {
|
||||
return `The user ran it on the ${spec.contextName}'s own page, at the version that page showed. ${ran}\n${outcome}`
|
||||
}
|
||||
return `${ran}${cleared}${reset}${stripped}${undeclared}\n${outcome}`
|
||||
}
|
||||
|
||||
|
||||
@@ -614,7 +614,9 @@ export type RunFormDisplay = {
|
||||
* a run the reader already started on the item's own page, which the tool follows instead. */
|
||||
export type RunFormAnswer =
|
||||
| { kind: 'args'; args: Record<string, any> }
|
||||
| { kind: 'job'; jobId: string }
|
||||
/** A job another surface already started. Its `args` are what that surface sent, already
|
||||
* coerced and minted by its own form, so they are reported rather than processed. */
|
||||
| { kind: 'job'; jobId: string; args: Record<string, any> }
|
||||
|
||||
export type RunFormDraft = {
|
||||
args: Record<string, any>
|
||||
|
||||
@@ -300,12 +300,6 @@
|
||||
// pressed under, and a call settled meanwhile — declined, then another proposed for
|
||||
// this same flow — must not be answered with a job it never asked for.
|
||||
const call = pendingRun
|
||||
// The page runs its own job, so nothing downstream gates this: the chat asked for the
|
||||
// run and then promised not to make one.
|
||||
if (call?.planModeActive) {
|
||||
sendUserToast(PLAN_MODE_MESSAGES.runFormRefused, true)
|
||||
return
|
||||
}
|
||||
loading = true
|
||||
const scheduledFor = scheduledForStr ? new Date(scheduledForStr).toISOString() : undefined
|
||||
try {
|
||||
@@ -321,7 +315,7 @@
|
||||
// The call asked for this run; it follows the job rather than starting one, and
|
||||
// shows it in place of this page — so navigating here as well would leave two
|
||||
// surfaces open on the same job.
|
||||
if (!call?.adopt(run)) await onNavigate('/run/' + run + '?workspace=' + workspace)
|
||||
if (!call?.adopt(run, args)) await onNavigate('/run/' + run + '?workspace=' + workspace)
|
||||
} catch (e) {
|
||||
throw e
|
||||
} finally {
|
||||
@@ -921,11 +915,13 @@
|
||||
bind:isValid
|
||||
runnable={flow}
|
||||
runAction={runFlow}
|
||||
argsReadonly={pendingRun?.planModeActive}
|
||||
runRefusal={pendingRun?.planModeActive
|
||||
? PLAN_MODE_MESSAGES.runFormRefused
|
||||
: undefined}
|
||||
bind:args
|
||||
schedulable={!pendingRun}
|
||||
bind:this={runForm}
|
||||
{jsonView}
|
||||
jsonView={jsonView && !pendingRun?.planModeActive}
|
||||
actions={promptForAi ? undefined : aiAssistant}
|
||||
/>
|
||||
</div>
|
||||
|
||||
@@ -389,12 +389,6 @@
|
||||
// pressed under, and a call settled meanwhile — declined, then another proposed for
|
||||
// this same script — must not be answered with a job it never asked for.
|
||||
const call = pendingRun
|
||||
// The page runs its own job, so nothing downstream gates this: the chat asked for the
|
||||
// run and then promised not to make one.
|
||||
if (call?.planModeActive) {
|
||||
sendUserToast(PLAN_MODE_MESSAGES.runFormRefused, true)
|
||||
return
|
||||
}
|
||||
try {
|
||||
runLoading = true
|
||||
const scheduledFor = scheduledForStr ? new Date(scheduledForStr).toISOString() : undefined
|
||||
@@ -423,7 +417,7 @@
|
||||
// The call asked for this run; it follows the job rather than starting one, and
|
||||
// shows it in place of this page — so navigating here as well would leave two
|
||||
// surfaces open on the same job.
|
||||
if (!call?.adopt(run)) await onNavigate('/run/' + run + '?workspace=' + workspace)
|
||||
if (!call?.adopt(run, args)) await onNavigate('/run/' + run + '?workspace=' + workspace)
|
||||
} catch (err) {
|
||||
sendUserToast(`Could not create job: ${err.body}`, true)
|
||||
} finally {
|
||||
@@ -1135,11 +1129,13 @@
|
||||
bind:isValid
|
||||
runnable={script}
|
||||
runAction={runScript}
|
||||
argsReadonly={pendingRun?.planModeActive}
|
||||
runRefusal={pendingRun?.planModeActive
|
||||
? PLAN_MODE_MESSAGES.runFormRefused
|
||||
: undefined}
|
||||
bind:args
|
||||
schedulable={!pendingRun}
|
||||
bind:this={runForm}
|
||||
{jsonView}
|
||||
jsonView={jsonView && !pendingRun?.planModeActive}
|
||||
actions={promptForAi ? undefined : aiAssistant}
|
||||
/>
|
||||
</div>
|
||||
|
||||
@@ -14,9 +14,11 @@ export type PendingRun = {
|
||||
/** What the chat proposed, for the form to open on. Read when the form is built, so this
|
||||
* is the live draft of the card's own form rather than a copy to keep in step. */
|
||||
readonly proposedArgs: Record<string, any>
|
||||
/** The job the reader started here answers the call. True when it did: the chat then shows
|
||||
* that run in place of this page, so the page must not navigate to it as well. False when
|
||||
* the call settled while the job was starting, and the run is the reader's alone. */
|
||||
adopt: (jobId: string) => boolean
|
||||
/** The job the reader started here answers the call, with the arguments it was started
|
||||
* with — the card has no other way to show what ran. True when the call took it: the chat
|
||||
* then shows that run in place of this page, so the page must not navigate to it as well.
|
||||
* False when the call settled while the job was starting, and the run is the reader's
|
||||
* alone. */
|
||||
adopt: (jobId: string, args: Record<string, any>) => boolean
|
||||
decline: () => void
|
||||
}
|
||||
|
||||
@@ -612,7 +612,7 @@ function createRuntime(session: Session): SessionRuntime {
|
||||
get proposedArgs() {
|
||||
return manager.pendingRunFormArgs(toolCallId) ?? {}
|
||||
},
|
||||
adopt: (jobId) => manager.handleRunFormJob(toolCallId, jobId),
|
||||
adopt: (jobId, args) => manager.handleRunFormJob(toolCallId, jobId, args),
|
||||
decline: () => manager.handleRunFormCancel(toolCallId)
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user