mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-10-12 16:02:56 +00:00
fix(sessions): address review round 2 on the deployed-run handover
- a seed consumed into the viewer's own state carried no item key, so a tab re-pointed to another item got the first one's arguments pushed into its form, replacing its defaults, under a note crediting the agent for them - the handover ran after a stopped turn: the tool reads the item's schema first, and a stop during that read settles the card, which the handover then overwrote with a form so the card read `Opened` and hid the stop. The manager owns the turn, so the check lives there and declining hands the call to requestRunArgs, whose own stop guard settles it - the card kept the deployed schema in the persisted transcript although it never mounts a form - a chat flow's non-message inputs now land whether or not the message does, and the result string no longer asserts a fill the chat branch may not have made - the callback's `false` is documented by what actually causes it Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
3ee5ca72f8
commit
3d80243963
@@ -570,12 +570,10 @@ export class AIChatManager implements ChatViewHost {
|
||||
* chat card holds. Unset outside a session: a chat-bound form has nowhere else to go,
|
||||
* so the card hides the control rather than offering a tab that cannot run. */
|
||||
openRunForm?: (a: { toolCallId: string; label: string }) => void
|
||||
/** A DEPLOYED run shows the item's own page instead, with the model's arguments in the
|
||||
* run form already there. The tool call ends at the open — the run belongs to the reader,
|
||||
* who starts it from that page with its scheduling, tag and version controls in reach.
|
||||
* Returns whether the panel took it; false (a legacy app has no in-panel page) leaves the
|
||||
* tool to fall back on its own form. */
|
||||
openDeployedRunPage?: (a: {
|
||||
/** Set by a host that can show a deployed page — a session's preview panel. Tools reach it
|
||||
* through {@link openDeployedRunPage}, never directly: the stop check belongs to the
|
||||
* manager, which owns the turn. */
|
||||
openDeployedRunPageHandler?: (a: {
|
||||
kind: 'script' | 'flow'
|
||||
path: string
|
||||
summary: string
|
||||
@@ -2100,6 +2098,25 @@ export class AIChatManager implements ChatViewHost {
|
||||
|
||||
/** False when the form is no longer pending, so the caller can say so instead of
|
||||
* leaving its submit button spinning on a run that will never start. */
|
||||
/** A DEPLOYED run shows the item's own page instead, with the model's arguments in the run
|
||||
* form already there. The tool call ends at the open — the run belongs to the reader, who
|
||||
* starts it from that page with its scheduling, tag and version controls in reach.
|
||||
*
|
||||
* False where there is no page to show (no session host) and where the turn was stopped
|
||||
* while the tool read the item's schema: opening then would act on a turn the reader
|
||||
* ended, and would write a form onto a card the stop already settled, which the card would
|
||||
* read as `Opened`. Declining hands the call on to `requestRunArgs`, whose own stop guard
|
||||
* settles it as cancelled. */
|
||||
openDeployedRunPage = (a: {
|
||||
kind: 'script' | 'flow'
|
||||
path: string
|
||||
summary: string
|
||||
args: Record<string, any>
|
||||
}): boolean => {
|
||||
if (this.abortController?.signal.aborted) return false
|
||||
return this.openDeployedRunPageHandler?.(a) ?? false
|
||||
}
|
||||
|
||||
handleRunFormSubmit = (toolId: string, args: Record<string, any>): boolean => {
|
||||
if (!this.isRunFormPending(toolId)) return false
|
||||
this.#settleRunForm(toolId, args)
|
||||
|
||||
@@ -6045,7 +6045,7 @@ async function testRunScriptByPath(
|
||||
* same reason as the cancelled one below — a re-proposal would reopen the page the reader is
|
||||
* already looking at. */
|
||||
const deployedPageOpened = (path: string, noun: string, toolName: string) =>
|
||||
`Opened the deployed page for "${path}" in the preview panel, with these arguments filled into its run form. The ${noun} has NOT run: the user starts it from that page, where they can also edit the arguments, schedule it, override the tag or pick a version. Do not call ${toolName} again for this. Tell the user it is ready to run; if you need the result afterwards, ask them or call get_run once they say it has run.`
|
||||
`Opened the deployed page for "${path}" in the preview panel, with these arguments put in front of the user there. The ${noun} has NOT run: the user starts it from that page, where they can also edit the arguments, schedule it, override the tag or pick a version. Do not call ${toolName} again for this. Tell the user it is ready to run; if you need the result afterwards, ask them or call get_run once they say it has run.`
|
||||
|
||||
/** The "do not call again" half is load-bearing: without it the model re-proposes the
|
||||
* call, which re-opens the form the user just dismissed, and Stop becomes their only
|
||||
@@ -6208,8 +6208,11 @@ async function runThroughForm(spec: FormRunSpec, ctx: WriteDraftCtx): Promise<st
|
||||
content: `Opened the deployed page for "${spec.path}"`,
|
||||
// Carried without `isLoading`, so `isActiveRunForm` reads it as settled: the card
|
||||
// shows what was proposed and offers to reopen the page, rather than mounting a
|
||||
// second form for a call that is already over.
|
||||
runForm: persisted,
|
||||
// second form for a call that is already over. Without the schema for the same
|
||||
// reason a submitted or cancelled form drops it — no form will mount from this,
|
||||
// so keeping it would persist the script's declarations, password and file
|
||||
// defaults included, for the life of the chat.
|
||||
runForm: { ...persisted, schema: undefined, code: undefined, lang: undefined },
|
||||
// The card settles on what the page opened with, as it would on what a form
|
||||
// opened with: it is the record of what this call proposed.
|
||||
parameters: persisted.args,
|
||||
|
||||
@@ -1420,8 +1420,13 @@ export interface ToolCallbacks {
|
||||
markRunFormStarted?: (toolId: string) => void
|
||||
/** Show a DEPLOYED run on the item's own page, with these arguments in the run form that
|
||||
* is already there, and end the call: the reader runs it from that page rather than
|
||||
* confirming a form the tool would then run itself. Returns whether the host took it.
|
||||
* Wired only where that page can be shown — a session's preview panel. */
|
||||
* confirming a form the tool would then run itself. Wired only where that page can be
|
||||
* shown — a session's preview panel.
|
||||
*
|
||||
* False means it did not open, and the caller must fall through to its own form: there is
|
||||
* no session to show it in, or the turn was stopped while this tool read the item's
|
||||
* schema. The form path settles a stopped turn correctly; this one has no card left to
|
||||
* settle. */
|
||||
openDeployedRunPage?: (a: {
|
||||
kind: 'script' | 'flow'
|
||||
path: string
|
||||
|
||||
@@ -374,28 +374,22 @@
|
||||
$effect(() => {
|
||||
if (!seededRun || seededSeq === seededRun.seq) return
|
||||
if (chatInputEnabled) {
|
||||
const message = seededRun.args?.user_message
|
||||
// Nothing to offer is settled, not pending: latch it so a flow whose schema has no
|
||||
// `user_message` does not re-ask on every flush.
|
||||
if (typeof message !== 'string' || !message) {
|
||||
seededSeq = seededRun.seq
|
||||
return
|
||||
}
|
||||
// The composer mounts a flush or two after the panel does, so until it is there this
|
||||
// effect waits — it reads the host's registration through `composerReady`, and runs
|
||||
// again when that lands. Not latching is what keeps the proposal alive across those
|
||||
// flushes.
|
||||
if (!flowChat?.composerReady()) return
|
||||
// Past that point the answer is final either way. The rest of the proposal goes with
|
||||
// the message: a chat flow's schema can declare more than `user_message`, and sending
|
||||
// the message alone would run on saved values or schema defaults — a different run
|
||||
// from the one being offered. A composer already holding a draft declines, and the
|
||||
// proposal is dropped rather than held: injecting it whenever the reader happens to
|
||||
// clear their draft would put words in the box long after they were proposed. The
|
||||
// card still shows what was asked for.
|
||||
const { user_message: _m, ...inputs } = seededRun.args ?? {}
|
||||
seededSeq = seededRun.seq
|
||||
flowChat.offerMessage(message, inputs)
|
||||
// A chat flow's schema can declare more than `user_message`, and a message sent
|
||||
// without the rest would run on saved values or schema defaults — a different run
|
||||
// from the one proposed. These land whether or not the message does.
|
||||
const { user_message: message, ...inputs } = seededRun.args ?? {}
|
||||
flowChat.applyInputs(inputs)
|
||||
// A composer already holding a draft declines, and the message is then dropped
|
||||
// rather than held: injecting it whenever the reader happens to clear their draft
|
||||
// would put words in the box long after they were proposed. The card still shows
|
||||
// what was asked for.
|
||||
if (typeof message === 'string' && message) flowChat.offerMessage(message)
|
||||
} else if (runForm) {
|
||||
seededSeq = seededRun.seq
|
||||
runForm.setArgs(seededRun.args)
|
||||
|
||||
@@ -186,11 +186,17 @@
|
||||
* `user_message`, and a message sent without them would run on saved values or schema
|
||||
* defaults — a different run from the one being offered. Applied only alongside a message
|
||||
* the composer took, so a declined offer leaves the reader's settings alone. */
|
||||
export function offerMessage(text: string, inputs?: Record<string, any>): boolean {
|
||||
export function offerMessage(text: string): boolean {
|
||||
const panel = shownKey ? pool?.get(shownKey) : undefined
|
||||
if (!panel?.host.offerMessage(text)) return false
|
||||
if (inputs) inputValues = { ...inputValues, ...inputs }
|
||||
return true
|
||||
return panel?.host.offerMessage(text) ?? false
|
||||
}
|
||||
|
||||
/** Set the flow's non-message inputs for the run a chat tool proposed. Separate from the
|
||||
* message because the two can land apart: a composer the reader is typing in declines the
|
||||
* message, and the inputs still belong to the proposal they can send afterwards. */
|
||||
export function applyInputs(inputs: Record<string, any>): void {
|
||||
if (Object.keys(inputs).length === 0) return
|
||||
inputValues = { ...inputValues, ...inputs }
|
||||
}
|
||||
|
||||
/** Whether the conversation on screen has a composer to offer a message to. A caller that
|
||||
|
||||
@@ -72,13 +72,20 @@
|
||||
// Taken from the runtime once and held here: the request belongs to the turn that made it,
|
||||
// so leaving it in the map would re-apply a stale proposal — and reannounce it — to every
|
||||
// later open of this item's deployed page.
|
||||
let seededRun = $state<{ args: Record<string, any>; seq: number } | undefined>(undefined)
|
||||
// Carries the item it was filed under: this tab re-points to another item in place rather
|
||||
// than remounting, so a seed held without its key would be pushed into the next item's
|
||||
// form — replacing its own defaults — under a note crediting the agent for them.
|
||||
let held = $state<
|
||||
{ kind: SessionTargetKind; path: string; args: Record<string, any>; seq: number } | undefined
|
||||
>(undefined)
|
||||
const seededRun = $derived(held?.kind === kind && held.path === path ? held : undefined)
|
||||
$effect(() => {
|
||||
const pending = runtime.seededRunArgsFor(kind, path)
|
||||
if (!pending) return
|
||||
const [k, p] = [kind, path]
|
||||
untrack(() => {
|
||||
seededRun = pending
|
||||
runtime.clearSeededRunArgs(kind, path)
|
||||
held = { kind: k, path: p, ...pending }
|
||||
runtime.clearSeededRunArgs(k, p)
|
||||
})
|
||||
})
|
||||
|
||||
@@ -221,7 +228,7 @@
|
||||
{active}
|
||||
embedded
|
||||
{seededRun}
|
||||
onClearSeededRun={() => (seededRun = undefined)}
|
||||
onClearSeededRun={() => (held = undefined)}
|
||||
onLoadState={setLoadState}
|
||||
/>
|
||||
{:else if kind === 'flow'}
|
||||
@@ -233,7 +240,7 @@
|
||||
{active}
|
||||
embedded
|
||||
{seededRun}
|
||||
onClearSeededRun={() => (seededRun = undefined)}
|
||||
onClearSeededRun={() => (held = undefined)}
|
||||
onLoadState={setLoadState}
|
||||
/>
|
||||
{:else}
|
||||
|
||||
@@ -616,10 +616,11 @@ function createRuntime(session: Session): SessionRuntime {
|
||||
// into the run form that is already there. Nothing waits on it: the tool call ends at the
|
||||
// open, and the run is the reader's, started from that page as it would be with no chat
|
||||
// open. `seq` rather than a flag, so a second request for a page already open re-seeds it.
|
||||
manager.openDeployedRunPage = ({ kind, path, summary, args }) => {
|
||||
manager.openDeployedRunPageHandler = ({ kind, path, summary, args }) => {
|
||||
// Through the shared adapter rather than building the item here: a session target
|
||||
// spells a code-based app `raw_app` and a workspace item spells it `app` with a
|
||||
// flag, and one place should know that.
|
||||
// flag, and one place should know that. A script and a flow both resolve to an item,
|
||||
// so this guard is the adapter's contract rather than a case that happens.
|
||||
const target = previewTargetForSessionTarget(kind, path)
|
||||
if (target?.type !== 'item') return false
|
||||
seededRunArgs.set(seededKey(kind, path), { args, seq: ++seededRunSeq })
|
||||
|
||||
Reference in New Issue
Block a user