From fe88cbe2b2beee6de010cc76a10c7b40cdb552bd Mon Sep 17 00:00:00 2001 From: Guilhem Lemouel Date: Thu, 10 Sep 2026 12:21:59 +0200 Subject: [PATCH] fix(ai-chat): only an agent that reads a flow input has a say in which one Four more from the CI review, and one regression of my own. An agent step carries a placeholder transform for every key of its schema, so "all agents must agree on the attachments input" read those placeholders as competing readers and any flow with two agents lost the composer's paperclip. An agent is a reader only when its transform actually references a flow input; anything else has no say. What still vetoes is the real ambiguity: two agents reading different inputs, an expression naming two, or a name the schema does not declare. The rest of that rule, applied where it was still missing: an agent whose provider expression cannot be read now voids the whole resolution rather than letting the others speak for it; the gap warning is asked of each agent, so one agent with an empty model still warns however well its neighbour is configured; the attachment cap counts what is queued, since a queue merges into one turn on flush; and the conversation a turn belongs to is read before its attachments upload, with the sidebar holding still meanwhile. `isWaitingForResponse` had no reset when a launch failed. That cost a stuck composer before; with the sidebar now holding on the same flag it would have cost a page reload, so a failed launch clears all three. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01QN7VboDEm9HAB1t4sMxMdE --- .../copilot/chat/AIChatInput.svelte | 6 +- .../conversations/FlowChatManager.svelte.ts | 28 +++++- .../FlowConversationsSidebar.svelte | 41 +++++--- .../conversations/agentChatInputs.test.ts | 74 ++++++++++++++- .../flows/conversations/agentChatInputs.ts | 95 ++++++++++++------- .../conversations/flowChatViewHost.svelte.ts | 6 +- 6 files changed, 198 insertions(+), 52 deletions(-) diff --git a/frontend/src/lib/components/copilot/chat/AIChatInput.svelte b/frontend/src/lib/components/copilot/chat/AIChatInput.svelte index 072a704f66..dc73ebc615 100644 --- a/frontend/src/lib/components/copilot/chat/AIChatInput.svelte +++ b/frontend/src/lib/components/copilot/chat/AIChatInput.svelte @@ -237,7 +237,11 @@ pendingFiles + draft.blobs.length + pendingBlobs - return Math.min(laneRemaining, Math.max(0, turnCap - staged)) + // A queue counts too: what is held mid-run merges into one turn on flush, so a + // second file accepted now would be dropped there instead of refused here. + const queued = + chatHost.queuedImages.length + chatHost.queuedFiles.length + chatHost.queuedBlobs.length + return Math.min(laneRemaining, Math.max(0, turnCap - staged - queued)) } /** What to say when the host's own limit is the one that bit. */ diff --git a/frontend/src/lib/components/flows/conversations/FlowChatManager.svelte.ts b/frontend/src/lib/components/flows/conversations/FlowChatManager.svelte.ts index eec40b11d3..bd6854ed1a 100644 --- a/frontend/src/lib/components/flows/conversations/FlowChatManager.svelte.ts +++ b/frontend/src/lib/components/flows/conversations/FlowChatManager.svelte.ts @@ -331,6 +331,13 @@ export class FlowChatManager { await this.#writeConversationTitle(conversationId, trimmed) } + /** No job came of the send, so nothing is in flight and nothing is waiting on one. */ + #turnFailedToStart() { + this.isLoading = false + this.isWaitingForResponse = false + this.isDispatchingTurn = false + } + /** A turn is being dispatched or is running: nothing may move the conversation under it. */ get isTurnInFlight(): boolean { return this.isLoading || this.isWaitingForResponse || this.isDispatchingTurn @@ -588,7 +595,16 @@ export class FlowChatManager { * An empty message is allowed here: a turn can carry attachments alone, and only the * caller knows whether it does. */ - async sendMessage(additionalInputs?: Record, onUserRow?: (rowId: string) => void) { + async sendMessage( + additionalInputs?: Record, + onUserRow?: (rowId: string) => void, + /** + * The conversation the turn was started in. Passed by a caller that had to await + * something first — an attachment upload — since the reader can select another + * conversation while it runs, and the turn belongs to the one they sent it from. + */ + pinnedConversationId?: string + ) { if (this.isLoading) return const isNewConversation = this.messages.length === 0 @@ -597,8 +613,8 @@ export class FlowChatManager { this.stopPolling() // Generate a new conversation ID if we don't have one - let currentConversationId = this.selectedConversationId - if (!this.selectedConversationId) { + let currentConversationId = pinnedConversationId ?? this.selectedConversationId + if (!currentConversationId) { const newConversationId = await this.createConversation({ clearMessages: false }) currentConversationId = newConversationId } @@ -649,6 +665,10 @@ export class FlowChatManager { } catch (error) { console.error('Error running flow:', error) sendUserToast('Failed to run flow: ' + error, true) + // A turn that never started leaves nothing to wait for. Said here as well as in + // the finally because the streaming path keeps `isLoading` for its own stream, + // and without this the composer and the sidebar stay locked until a reload. + this.#turnFailedToStart() } finally { if (!this.#useStreaming) { this.isLoading = false @@ -692,6 +712,7 @@ export class FlowChatManager { const jobId = await this.#onRunFlow?.(messageContent, currentConversationId, additionalInputs) if (!jobId) { console.error('No jobId returned from onRunFlow') + this.#turnFailedToStart() return } @@ -829,6 +850,7 @@ export class FlowChatManager { const jobId = await this.#onRunFlow?.(messageContent, currentConversationId, additionalInputs) if (!jobId) { console.error('No jobId returned from onRunFlow') + this.#turnFailedToStart() return } diff --git a/frontend/src/lib/components/flows/conversations/FlowConversationsSidebar.svelte b/frontend/src/lib/components/flows/conversations/FlowConversationsSidebar.svelte index e6b8137eb7..db460cde3f 100644 --- a/frontend/src/lib/components/flows/conversations/FlowConversationsSidebar.svelte +++ b/frontend/src/lib/components/flows/conversations/FlowConversationsSidebar.svelte @@ -83,6 +83,11 @@ all: 'All' } + /** A turn is running and this is not the chat it is running in. */ + function rowLocked(conversation: ConversationWithDraft): boolean { + return manager.isTurnInFlight && manager.selectedConversationId !== conversation.id + } + function getConversationTitle(conversation: FlowConversation): string { return conversation.title || `Conversation ${conversation.created_at.slice(0, 10)}` } @@ -122,7 +127,10 @@ variant="subtle" startIcon={{ icon: Plus, classes: 'ml-[2px]' }} onClick={() => manager.createConversation({ clearMessages: true })} - title="Start new conversation" + disabled={manager.isTurnInFlight} + title={manager.isTurnInFlight + ? 'Wait for the current answer to start a new chat' + : 'Start new conversation'} iconOnly={!manager.isSidebarExpanded} wrapperClasses={manager.isSidebarExpanded ? 'grow min-w-0' : ''} btnClasses={'w-full justify-start transition-all duration-150 whitespace-nowrap'} @@ -245,6 +253,10 @@ variant="subtle" onClick={() => manager.selectConversation(conversation.id, conversation.isDraft)} selected={manager.selectedConversationId === conversation.id} + disabled={rowLocked(conversation)} + title={rowLocked(conversation) + ? 'Wait for the current answer to switch conversation' + : undefined} btnClasses="transition-all duration-150 group" > {#if conversation.is_test} @@ -255,17 +267,22 @@ {getConversationTitle(conversation)} - - -
e.stopPropagation()} - > - rowActions(conversation)} size="xs" /> -
+ + {#if !rowLocked(conversation)} + + +
e.stopPropagation()} + > + rowActions(conversation)} size="xs" /> +
+ {/if} {/if} diff --git a/frontend/src/lib/components/flows/conversations/agentChatInputs.test.ts b/frontend/src/lib/components/flows/conversations/agentChatInputs.test.ts index 3bc4c6d119..bb848c3bd2 100644 --- a/frontend/src/lib/components/flows/conversations/agentChatInputs.test.ts +++ b/frontend/src/lib/components/flows/conversations/agentChatInputs.test.ts @@ -1,7 +1,20 @@ import { describe, expect, it } from 'vitest' -import { agentModelGap, parseProviderTransform, resolveAgentModelWiring } from './agentChatInputs' +import { + agentModelGap, + parseProviderTransform, + resolveAgentChatInputs, + resolveAgentModelWiring +} from './agentChatInputs' import type { FlowModule } from '$lib/gen' +/** An agent step with the given input transforms, as the editor stores them. */ +function agentWith(input_transforms: Record): FlowModule { + return { + id: 'a', + value: { type: 'aiagent', tools: [], input_transforms } + } as unknown as FlowModule +} + function agent(expr: string): FlowModule { return { id: 'a', @@ -89,6 +102,34 @@ describe('parseProviderTransform', () => { }) }) +describe('resolveAgentChatInputs', () => { + const schema = { properties: { files: { type: 'array' } }, required: [] } + const reader = (name: string) => + agentWith({ user_attachments: { type: 'javascript', expr: `flow_input.${name}` } }) + // Every agent step carries a placeholder transform for each key of AI_AGENT_SCHEMA + // (loadSchemaFromModule writes them back onto the module), so an agent that reads + // nothing must not be mistaken for one reading a different input. + const seeded = () => agentWith({ user_attachments: { type: 'static', value: undefined } }) + + it('promotes the input one agent reads', () => { + expect(resolveAgentChatInputs([reader('files')], schema).map((i) => i.name)).toEqual(['files']) + }) + + it('still promotes it when another agent leaves the field unwired', () => { + expect(resolveAgentChatInputs([reader('files'), seeded()], schema).map((i) => i.name)).toEqual([ + 'files' + ]) + }) + + it('promotes nothing when two agents read different inputs', () => { + const twoInputs = { + properties: { files: { type: 'array' }, docs: { type: 'array' } }, + required: [] + } + expect(resolveAgentChatInputs([reader('files'), reader('docs')], twoInputs)).toEqual([]) + }) +}) + describe('resolveAgentModelWiring', () => { const fixedResource = `"kind": "anthropic", "resource": "$res:u/admin/claude"` @@ -132,7 +173,12 @@ describe('resolveAgentModelWiring', () => { } } ] - expect(resolveAgentModelWiring([parent])).toEqual({ whole: 'model', fields: {}, fixed: {} }) + expect(resolveAgentModelWiring([parent])).toEqual({ + whole: 'model', + fields: {}, + fixed: {}, + someAgentCannotRun: false + }) }) // The control writes one flow input; an agent that fixes the field instead never reads @@ -144,7 +190,6 @@ describe('resolveAgentModelWiring', () => { ]) expect(wiring?.fields.model).toBeUndefined() expect(wiring?.fixed.model).toBeUndefined() - expect(wiring?.undecided).toContain('model') }) // Disagreeing about the model is not the same as having no model: the flow runs, on a @@ -163,6 +208,29 @@ describe('resolveAgentModelWiring', () => { ).toBe('Pick a provider and model on the AI agent step to use this chat.') }) + // An expression the parser cannot account for could supply anything, so the agents it + // belongs to cannot be spoken for either. + it("offers nothing when one agent's provider cannot be read", () => { + expect( + resolveAgentModelWiring([ + agent(`({ ${fixedResource}, model: flow_input.model })`), + agent(`({ ...base, model: flow_input.model })`) + ]) + ).toBeUndefined() + }) + + // Disagreement is not the same as absence, but an agent with an empty model still + // cannot run, however well its neighbour is configured. + it('keeps warning when one agent has no model and another does', () => { + const wiring = resolveAgentModelWiring([ + agent(`({ ${fixedResource}, "model": "claude-sonnet-5" })`), + agent(`({ ${fixedResource}, "model": "" })`) + ]) + expect(agentModelGap(wiring)).toBe( + 'Pick a provider and model on the AI agent step to use this chat.' + ) + }) + it('refuses a flow mixing whole-object and field-by-field wiring', () => { expect( resolveAgentModelWiring([ diff --git a/frontend/src/lib/components/flows/conversations/agentChatInputs.ts b/frontend/src/lib/components/flows/conversations/agentChatInputs.ts index 8b3c02c9be..91f6231097 100644 --- a/frontend/src/lib/components/flows/conversations/agentChatInputs.ts +++ b/frontend/src/lib/components/flows/conversations/agentChatInputs.ts @@ -107,12 +107,11 @@ export type AgentModelWiring = { fields: Partial> fixed: Partial> /** - * Fields the agents supply differently from one another — one reading an input where - * another writes a literal, or two literals that disagree. Neither editable nor known, - * and told apart from a field nobody supplies at all: a control offered here would - * govern one agent while the rest ran on something else. + * One of the agents names no resource or no model of its own and no flow input feeds + * it, so that agent's run fails whatever the others do. Held apart from the fields, + * which describe what the composer may offer. */ - undecided?: ProviderField[] + someAgentCannotRun?: boolean } /** The flow input behind `flow_input.x`, `flow_input?.x` or `flow_input['x']`. */ @@ -200,6 +199,13 @@ type FieldSupply = | { kind: 'fixed'; value: any } | { kind: 'absent' } +/** Whether one agent supplies a field with nothing usable: no input, and no literal. */ +function agentFieldEmpty(wiring: AgentModelWiring, field: ProviderField): boolean { + if (wiring.fields[field] !== undefined) return false + const value = wiring.fixed[field] + return value === undefined || value === '' +} + function fieldSupply(wiring: AgentModelWiring, field: ProviderField): FieldSupply { const name = wiring.fields[field] if (name !== undefined) return { kind: 'wired', name } @@ -219,35 +225,44 @@ function fieldSupply(wiring: AgentModelWiring, field: ProviderField): FieldSuppl export function resolveAgentModelWiring( modules: FlowModule[] | undefined ): AgentModelWiring | undefined { - const wirings = agentSteps(modules) - .map((agent) => parseProviderTransform((agent.value as any).input_transforms?.['provider'])) - .filter((wiring): wiring is AgentModelWiring => wiring !== undefined) - if (wirings.length === 0) return undefined - if (wirings.length === 1) return wirings[0] + const agents = agentSteps(modules) + const parsed = agents.map((agent) => + parseProviderTransform((agent.value as any).input_transforms?.['provider']) + ) + if (parsed.length === 0) return undefined + // An agent whose provider cannot be read is an agent the composer cannot speak for: + // dropping it would let the rest declare a control that governs only some of them. + if (parsed.some((wiring) => wiring === undefined)) return undefined + const wirings = parsed as AgentModelWiring[] + // Whether any single agent has nothing to call, which stays true however the others + // are wired — the gap message is about that agent, not about their agreement. + const someAgentCannotRun = wirings.some( + (wiring) => + !wiring.whole && (agentFieldEmpty(wiring, 'resource') || agentFieldEmpty(wiring, 'model')) + ) + if (wirings.length === 1) return { ...wirings[0], someAgentCannotRun } const wholes = new Set(wirings.map((w) => w.whole)) if (wholes.size === 1 && !wholes.has(undefined)) { - return { whole: [...wholes][0], fields: {}, fixed: {} } + return { whole: [...wholes][0], fields: {}, fixed: {}, someAgentCannotRun } } if (wirings.some((w) => w.whole !== undefined)) return undefined const fields: AgentModelWiring['fields'] = {} const fixed: AgentModelWiring['fixed'] = {} - const undecided: ProviderField[] = [] for (const field of PROVIDER_FIELDS) { // Every agent has to supply the field the same way for the composer to speak for // them all. One wired name among agents that otherwise fix it is not agreement: // the control would move that one agent and leave the others where they are. const supplies = new Set(wirings.map((w) => JSON.stringify(fieldSupply(w, field)))) - if (supplies.size > 1) { - undecided.push(field) - continue - } + // Disagreement leaves the field neither editable nor known: a control offered here + // would govern one agent while the rest ran on something else. + if (supplies.size > 1) continue const supply: FieldSupply = JSON.parse([...supplies][0]) if (supply.kind === 'wired') fields[field] = supply.name else if (supply.kind === 'fixed') fixed[field] = supply.value } - return { fields, fixed, undecided } + return { fields, fixed, someAgentCannotRun } } /** @@ -261,15 +276,10 @@ export function resolveAgentModelWiring( export function agentModelGap(wiring: AgentModelWiring | undefined): string | undefined { // No agent, several of them, or an expression we cannot read: not ours to judge. if (!wiring || wiring.whole) return undefined - // Agents that disagree about what to call are not agents with nothing to call: the flow - // may well run, on a different model per agent, and this message would be false. - if (wiring.undecided?.some((field) => field === 'resource' || field === 'model')) { - return undefined - } - const missing = (field: ProviderField) => - wiring.fields[field] === undefined && - (wiring.fixed[field] === undefined || wiring.fixed[field] === '') - return missing('resource') || missing('model') + // Asked of each agent rather than of what they agree on: agents that merely disagree + // about the model all have one, and the message would be false — while an agent with + // an empty model still cannot run, however well the others are configured. + return wiring.someAgentCannotRun ? 'Pick a provider and model on the AI agent step to use this chat.' : undefined } @@ -300,18 +310,39 @@ export function resolveAgentChatInputs( ? additionalInputsSchema.required : [] - const keyOf = new Map() + // One input per key, and only when every agent reading that key reads the same one: + // the composer writes a single flow input, so promoting one of two would feed one + // agent and leave the other with nothing — while hiding both from the modal, where + // the reader could at least have filled them in. + const namesPerKey = new Map>() for (const module of agentSteps(modules)) { const transforms = (module.value as any).input_transforms ?? {} for (const key of AGENT_CHAT_INPUT_KEYS) { - const name = flowInputRef(transforms[key]) - // A name the schema doesn't declare has no field to promote, and `user_message` - // is already the composer itself. - if (!name || !(name in properties) || keyOf.has(name)) continue - keyOf.set(name, key) + const transform = transforms[key] + // An agent that feeds the key from anything but a flow input — a literal, another + // step's result, or the empty placeholder every agent step carries for the keys of + // AI_AGENT_SCHEMA — is not reading an input, so it has no say in which one the + // composer drives. + if (transform?.type !== 'javascript' || !transform.expr.includes('flow_input')) continue + const name = flowInputRef(transform) + // A name the schema doesn't declare has no field to promote, and one expression + // reading two inputs names none: either way this agent reads something the + // composer cannot drive, which is what disagreement means here. + const usable = name && name in properties ? name : undefined + const names = namesPerKey.get(key) ?? new Set() + names.add(usable) + namesPerKey.set(key, names) } } + const keyOf = new Map() + for (const [key, names] of namesPerKey) { + if (names.size !== 1) continue + const name = [...names][0] + if (name === undefined || keyOf.has(name)) continue + keyOf.set(name, key) + } + return [...keyOf.entries()] .map(([name, key]) => ({ name, diff --git a/frontend/src/lib/components/flows/conversations/flowChatViewHost.svelte.ts b/frontend/src/lib/components/flows/conversations/flowChatViewHost.svelte.ts index ebf972d133..81a939f72e 100644 --- a/frontend/src/lib/components/flows/conversations/flowChatViewHost.svelte.ts +++ b/frontend/src/lib/components/flows/conversations/flowChatViewHost.svelte.ts @@ -280,6 +280,9 @@ export class FlowChatViewHost implements ChatViewHost { ) } const attachments = [...images, ...blobs] + // Read before the upload below: the reader can pick another chat while it runs, and + // the turn belongs to the one they sent it from. + const conversationId = this.#manager.selectedConversationId let sentInputs: MessageInputs | undefined if (target && attachments.length > 0) { this.#uploading = true @@ -315,7 +318,8 @@ export class FlowChatViewHost implements ChatViewHost { Object.entries(this.#sentInputs).filter(([id]) => live.has(id)) ) this.#sentInputs = { ...kept, [rowId]: sentInputs } - } + }, + conversationId ) return true }