diff --git a/server/src/nest/assistant/assistant-agent.ts b/server/src/nest/assistant/assistant-agent.ts index 693f1bc86..caec30b6a 100644 --- a/server/src/nest/assistant/assistant-agent.ts +++ b/server/src/nest/assistant/assistant-agent.ts @@ -264,8 +264,14 @@ interface Turn { history: AiChatMessage[]; usage: { inputTokens: number; cachedInputTokens: number; outputTokens: number }; knownPlaceIds: Set; - /** Whether the turn used its one push (an empty, cut-off, announcing or wordless answer). */ + /** Whether the turn used its one push (an empty, cut-off or announcing answer). */ nudged: boolean; + /** + * Whether the turn already asked for the answer after a step that only + * offered follow-ups. Apart from the push, so a turn that was pushed once + * still gets its answer written (#2789); each is asked at most once. + */ + askedForAnswer: boolean; /** The chips of follow-ups offered before any answer, shown under the answer once it is written. */ followUps: AssistantFollowUps | null; } @@ -528,7 +534,7 @@ function answeredAsk(call: AiToolCall): AssistantMessagePart { */ async function askForTheAnswer(turn: Turn, rowId: number, step: StepRecord & { asks: AiToolCall[] }): Promise { const { asks, providerState } = step; - turn.nudged = true; + turn.askedForAnswer = true; turn.followUps = followUpsOf(asks[asks.length - 1].args); const parts = [...step.parts, ...asks.map((call) => ({ ...answeredAsk(call), result: WRITE_THE_ANSWER }))]; await turn.deps.store.saveStep(rowId, { parts, providerState }, 'complete'); @@ -560,12 +566,15 @@ async function runStep(turn: Turn, step: number): Promise { ...searched.tail, ]; const empty = received.length === 0 && !text.trim(); - if (empty && asks.length > 0 && !turn.nudged) return askForTheAnswer(turn, rowId, { parts, providerState, asks }); + if (empty && asks.length > 0 && !turn.askedForAnswer) + return askForTheAnswer(turn, rowId, { parts, providerState, asks }); // An answer that offered follow-ups is finished, however it ends. const settled = asks.length > 0 && !empty; const announced = !settled && received.length === 0 && step === 0 && ANNOUNCED.test(text.trimEnd()); const push = empty || (!settled && received.length === 0 && cutOff) ? GO_ON : announced ? MAKE_THE_CALLS : null; - if (push && !turn.nudged) { + // Follow-ups with no answer a second time end the turn: the model was already asked for the answer. + const wordlessAgain = empty && asks.length > 0; + if (push && !turn.nudged && !wordlessAgain) { turn.nudged = true; await deps.store.saveStep(rowId, { parts, providerState }, 'complete'); w.finishStep(); @@ -663,6 +672,7 @@ export async function runAgentLoop( usage: { inputTokens: 0, cachedInputTokens: 0, outputTokens: 0 }, knownPlaceIds, nudged: false, + askedForAnswer: false, followUps: null, }; return stepsFrom(turn, 0); diff --git a/server/src/nest/assistant/assistant-history.ts b/server/src/nest/assistant/assistant-history.ts index 876a43bdc..bae89f757 100644 --- a/server/src/nest/assistant/assistant-history.ts +++ b/server/src/nest/assistant/assistant-history.ts @@ -4,6 +4,7 @@ import type { AssistantToolCallRow } from '../../db/repositories/AssistantToolCa import { parseDbTimestamp, utcSuffix } from '../../db/types'; import { decodeJson } from '../../utils/json-column'; import type { AiChatMessage, AiProviderState, AiToolCall } from '../ai-connections/ai-chat-client.interface'; +import { asRecord, asText, parseToolArgs } from '../ai-connections/providers/provider-http'; import { tourFileOutcome, tourFileReadNote, tourStopOutcome } from './assistant-history-tours'; import { fitToSize } from './assistant-output'; import { @@ -48,17 +49,23 @@ const NOT_RUN = 'This call did not run: the answer was stopped before it.'; * The calls a provider's own record of a step holds, which it replays as they * came and wants answered: Anthropic's tool uses, the Responses API's function * calls. A step stopped while its calls ran (the user left the page) keeps the - * whole record but answers only the calls that ran. + * whole record but answers only the calls that ran. The arguments come along, + * so an adapter that rebuilds the step from its calls (another provider than + * the one that recorded it) still finds every call an answer names. */ -function recordedCalls(state: AiProviderState | null): Array<{ id: string; name: string }> { - const entries = +function recordedCalls(state: AiProviderState | null): AiToolCall[] { + const calls = state?.adapter === 'anthropic-messages' - ? state.blocks.filter((block) => block.type === 'tool_use').map((block) => [block.id, block.name]) + ? state.blocks + .filter((block) => block.type === 'tool_use') + .map((block) => ({ id: block.id, name: block.name, args: asRecord(block.input) })) : state?.adapter === 'openai-responses' - ? state.items.filter((item) => item.type === 'function_call').map((item) => [item.call_id, item.name]) + ? state.items + .filter((item) => item.type === 'function_call') + .map((item) => ({ id: item.call_id, name: item.name, args: parseToolArgs(asText(item.arguments)) })) : []; - return entries.flatMap(([id, name]) => - typeof id === 'string' ? [{ id, name: typeof name === 'string' ? name : '' }] : [], + return calls.flatMap(({ id, name, args }) => + typeof id === 'string' ? [{ id, name: typeof name === 'string' ? name : '', args }] : [], ); } @@ -314,7 +321,16 @@ export function toChatHistory( const text = textOf(parts); if (!text && toolCalls.length === 0) return; const providerState = decodeJson(ASSISTANT_PROVIDER_STATE, row.provider_state, `message ${row.id}`); - history.push({ role: 'assistant', text, toolCalls, providerState }); + // The recorded calls that never ran go into the step's calls as well: an adapter that does not + // replay this record rebuilds the step from them, and would otherwise find answers to calls it never made. + const answered = new Set(toolCalls.map((call) => call.id)); + const unrun: AiToolCall[] = []; + for (const recorded of recordedCalls(providerState)) { + if (answered.has(recorded.id)) continue; + answered.add(recorded.id); + unrun.push(recorded); + } + history.push({ role: 'assistant', text, toolCalls: [...toolCalls, ...unrun], providerState }); calls.forEach((part, index) => { history.push({ role: 'tool', @@ -324,11 +340,8 @@ export function toChatHistory( ...(part.isError ? { isError: true } : {}), }); }); - const answered = new Set(toolCalls.map((call) => call.id)); - for (const recorded of recordedCalls(providerState)) { - if (answered.has(recorded.id)) continue; - answered.add(recorded.id); - history.push({ role: 'tool', toolCallId: recorded.id, name: recorded.name, output: NOT_RUN, isError: true }); + for (const call of unrun) { + history.push({ role: 'tool', toolCallId: call.id, name: call.name, output: NOT_RUN, isError: true }); } }); return history; diff --git a/server/src/nest/assistant/assistant-tools.service.ts b/server/src/nest/assistant/assistant-tools.service.ts index 598d28f3e..73e0a1745 100644 --- a/server/src/nest/assistant/assistant-tools.service.ts +++ b/server/src/nest/assistant/assistant-tools.service.ts @@ -266,9 +266,12 @@ export class AssistantToolSession { /** The groups this turn loaded; the core is always there. */ private readonly loaded = new Set(); - /** Tools the model called before their group was loaded in this turn, by name, with that group. */ + /** Reads the model called before their group was loaded in this turn, by name, with that group; told once. */ private readonly calledBlind = new Map(); + /** Groups a blind call loaded whose schemas no step has been offered yet. */ + private readonly unoffered = new Set(); + /** Whether the session offers its tools by group: a trip's turn and a home turn do. */ private get grouped(): boolean { return this.mode === 'turn' || this.mode === 'home'; @@ -280,6 +283,8 @@ export class AssistantToolSession { * journey's session offers all it holds and no more. */ specs(): AiToolSpec[] { + // The step these specs go to sees every loaded group's schema. + this.unoffered.clear(); const listed = [...this.tools.values()] .filter((tool) => tool.offered) .filter((tool) => !this.grouped || tool.hint.group === null || this.loaded.has(tool.hint.group)) @@ -338,9 +343,25 @@ export class AssistantToolSession { // The model called a tool it was not offered in this turn (it knows the // name from an earlier turn), so it wrote the arguments without the // schema (#2967). Its group is loaded here, so the next step offers the - // schema, and a refusal of these arguments says so (see call()). + // schema. this.loaded.add(unloaded); - this.calledBlind.set(name, unloaded); + this.unoffered.add(unloaded); + } + const group = this.tools.get(name)?.hint.group; + if (group && this.unoffered.has(group)) { + // A change is never run in the turn but held as a card, and the turn + // ends there: its arguments would only fail once the user applies it, + // with the model long gone. It is sent back for the next step instead. + if (this.tools.get(name)?.kind === 'write') { + return { + error: `${name} belongs to the "${group}" tools, which were not loaded in this turn, so it did not run. They are loaded now: send the call again with the arguments exactly as its schema describes them.`, + }; + } + // A read runs at once; a refusal of its arguments says why (see call()). + this.calledBlind.set(name, group); + } else { + // A blind read refused before it ran (another trip, an unknown place) must not hint a later call. + this.calledBlind.delete(name); } const given = pinNamesOf(this.pinArg).filter((key) => args[key] !== undefined); if (given.some((key) => Number(args[key]) !== this.pinnedId)) { @@ -361,7 +382,10 @@ export class AssistantToolSession { const final = name === TRANSIT_CREATE_TOOL ? await this.withFoundItinerary(args) : args; const run = await this.invoke(name, final); this.audit(name); + // Only the blind call itself is told about the schema: a later failure of the + // same tool, written with the schema in hand, has another reason. const blind = this.calledBlind.get(name); + this.calledBlind.delete(name); if (!run.ok && blind) { const hint = `${name} was called before the "${blind}" tools were loaded in this turn, so its arguments were written without its schema. They are loaded now and the schema is in your tools: send the call again with the arguments exactly as it describes them.`; return { diff --git a/server/tests/unit/nest/assistant/assistant-agent.test.ts b/server/tests/unit/nest/assistant/assistant-agent.test.ts index 83edc1736..4326f873d 100644 --- a/server/tests/unit/nest/assistant/assistant-agent.test.ts +++ b/server/tests/unit/nest/assistant/assistant-agent.test.ts @@ -859,6 +859,49 @@ describe('runAgentLoop', () => { expect(outcome).toMatchObject({ end: 'failed', code: 'empty_answer' }); expect(t.requests).toHaveLength(2); }); + + it('ASTAGT-045: follow-ups with no answer after an announcing first answer are still asked for the answer', async () => { + const t = setup([ + [{ type: 'text-delta', delta: 'Let me check your places:' }], + [{ type: 'tool-call', call: { id: 'l1', name: 'list_places', args: {} } }], + [ask('toolu_8', ['Add Ginkaku-ji']), { type: 'finish', reason: 'tool-calls' }], + [{ type: 'text-delta', delta: 'Kinkaku-ji is the only place so far.' }], + ]); + const outcome = await runAgentLoop(t.deps, t.input, t.writer, new AbortController().signal); + expect(outcome.end).toBe('stop'); + expect(t.requests).toHaveLength(4); + expect(t.requests[1].messages.at(-1)).toMatchObject({ role: 'user', text: expect.stringContaining('now') }); + expect(t.requests[3].messages.at(-1)).toMatchObject({ + role: 'tool', + toolCallId: 'toolu_8', + output: expect.stringContaining('Not shown yet'), + }); + expect(t.chunks).toContainEqual({ type: 'data-trek-follow-ups', data: { questions: ['Add Ginkaku-ji'] } }); + expect(t.memory.saved.map((saved) => saved.status)).not.toContain('error'); + }); + }); + + it('ASTAGT-046: a change its session sends back (called before its tools were loaded) is a tool error, never held (#2967)', async () => { + const t = setup([ + [{ type: 'tool-call', call: { id: 'w1', name: 'create_place', args: { name: 'Blind' } } }], + [{ type: 'tool-call', call: { id: 'w2', name: 'create_place', args: { name: 'With schema' } } }], + ]); + t.tools.pinArgs.mockImplementationOnce(() => ({ error: 'create_place did not run: send it again' })); + const outcome = await runAgentLoop(t.deps, t.input, t.writer, new AbortController().signal); + expect(outcome.end).toBe('awaiting'); + expect(t.memory.saved[0].step.parts).toContainEqual({ + type: 'tool-call', + toolCallId: 'w1', + toolName: 'create_place', + args: { name: 'Blind' }, + result: 'create_place did not run: send it again', + isError: true, + }); + const sentBack = t.requests[1].messages.find((m) => m.role === 'tool' && m.toolCallId === 'w1'); + expect(sentBack).toMatchObject({ isError: true, output: expect.stringContaining('did not run') }); + expect(t.deps.describe).toHaveBeenCalledTimes(1); + expect(t.memory.held).toHaveLength(1); + expect(t.memory.held[0].held.map((call) => call.args)).toEqual([{ name: 'With schema' }]); }); it('ASTAGT-038: followUpsOf trims, drops duplicates and blanks, cuts each to a chip and keeps three', () => { diff --git a/server/tests/unit/nest/assistant/assistant-history.test.ts b/server/tests/unit/nest/assistant/assistant-history.test.ts index 4f643e06a..ab7a12038 100644 --- a/server/tests/unit/nest/assistant/assistant-history.test.ts +++ b/server/tests/unit/nest/assistant/assistant-history.test.ts @@ -651,6 +651,61 @@ describe('toChatHistory: calls the model made of the server tools (#2845)', () = expect(outputs).toEqual(['call_1', 'call_2']); }); + it('ASTHIS-021: a stopped step replayed on another provider names every call it answers', () => { + const anthropic = stoppedMidStep({ + adapter: 'anthropic-messages', + blocks: [ + { type: 'text', text: 'Let me look.' }, + { type: 'tool_use', id: 'call_1', name: 'list_places', input: {} }, + { type: 'tool_use', id: 'call_2', name: 'get_weather', input: { day: 3 } }, + ], + }); + expect(anthropic.find((message) => message.role === 'assistant')).toMatchObject({ + toolCalls: [ + { id: 'call_1', name: 'list_places', args: {} }, + { id: 'call_2', name: 'get_weather', args: { day: 3 } }, + ], + }); + + // Chat Completions: every tool message follows an assistant message whose tool_calls name it. + const chat = toChatMessages({ system: '', messages: anthropic, tools: [] }); + chat.forEach((message, index) => { + if (message.role !== 'tool') return; + const before = chat.slice(0, index).reverse().find((earlier) => earlier.role === 'assistant'); + const named = ((before?.tool_calls ?? []) as Array<{ id: string }>).map((call) => call.id); + expect(named).toContain(message.tool_call_id); + }); + + // Responses: every function_call_output has its function_call before it. + const items = toResponsesInput({ system: '', messages: anthropic, tools: [] }); + items.forEach((item, index) => { + if (item.type !== 'function_call_output') return; + const made = items + .slice(0, index) + .some((earlier) => earlier.type === 'function_call' && earlier.call_id === item.call_id); + expect(made).toBe(true); + }); + + // Anthropic: a step the Responses API recorded is rebuilt with every tool_use its results answer. + const responses = stoppedMidStep({ + adapter: 'openai-responses', + items: [ + { type: 'function_call', call_id: 'call_1', name: 'list_places', arguments: '{}' }, + { type: 'function_call', call_id: 'call_2', name: 'get_weather', arguments: 'not json' }, + ], + }); + const messages = toAnthropicMessages(responses); + const uses = messages[1].content.filter((block) => block.type === 'tool_use'); + expect(uses.map((block) => [block.id, block.input])).toEqual([ + ['call_1', {}], + ['call_2', {}], + ]); + const results = messages[2].content + .filter((block) => block.type === 'tool_result') + .map((block) => block.tool_use_id); + expect(results).toEqual(['call_1', 'call_2']); + }); + it('ASTHIS-017: a step that only kept a note is the model call it was, answered with what was kept', () => { const history = toChatHistory( [ diff --git a/server/tests/unit/nest/assistant/assistant-tools.service.test.ts b/server/tests/unit/nest/assistant/assistant-tools.service.test.ts index 8722cf0a8..0e20794aa 100644 --- a/server/tests/unit/nest/assistant/assistant-tools.service.test.ts +++ b/server/tests/unit/nest/assistant/assistant-tools.service.test.ts @@ -425,6 +425,63 @@ describe('an assistant tool session', () => { } }); + it('ASTTOOL-025: a change called before its group was loaded is sent back until a step was offered its schema (#2967)', async () => { + const { userId, tripId } = await ownerWithTrips(); + const session = await tools.open(userId, tripId); + try { + expect(session.specs().map((spec) => spec.name)).not.toContain('insert_tour_stop'); + // Held as a card, the blind arguments would only fail once the user applies it. + expect(session.pinArgs('insert_tour_stop', { placeId: '530' })).toEqual({ + error: expect.stringContaining('"tours" tools, which were not loaded in this turn, so it did not run'), + }); + // A second change of the same step was written without the schema too. + expect(session.pinArgs('create_city_walk', { stations: '[1, 2]' })).toEqual({ + error: expect.stringContaining('did not run'), + }); + // The next step is offered the schema, and the call goes through. + expect(session.specs().map((spec) => spec.name)).toContain('insert_tour_stop'); + expect(session.pinArgs('insert_tour_stop', { placeId: 530 })).toHaveProperty('args'); + } finally { + await session.close(); + } + }); + + it('ASTTOOL-026: only the blind call itself is told about the schema; a later failure of the same tool is not', async () => { + const { userId, tripId } = await ownerWithTrips(); + const session = await tools.open(userId, tripId); + try { + session.specs(); + expect(session.pinArgs('get_tour_plan', { placeId: '530' })).toEqual({ args: { placeId: '530' } }); + const blind = await session.call('get_tour_plan', { placeId: '530' }); + expect(String(blind.value)).toContain('"tours" tools were loaded in this turn'); + // Sent again after the next step's specs: it fails on its own, without the hint. + session.specs(); + expect(session.pinArgs('get_tour_plan', { placeId: 999_999 })).toEqual({ args: { placeId: 999_999 } }); + const again = await session.call('get_tour_plan', { placeId: 999_999 }); + expect(again.ok).toBe(false); + expect(String(again.value)).not.toContain('were loaded in this turn'); + } finally { + await session.close(); + } + }); + + it('ASTTOOL-027: a blind read refused before it ran leaves no hint for a later call of the same tool', async () => { + const { userId, tripId } = await ownerWithTrips(); + const session = await tools.open(userId, tripId); + try { + session.specs(); + // Called blind, then refused by the loop before it ran: call() never sees it. + expect(session.pinArgs('get_tour_plan', { placeId: '530' })).toHaveProperty('args'); + session.specs(); + expect(session.pinArgs('get_tour_plan', { placeId: 999_999 })).toEqual({ args: { placeId: 999_999 } }); + const run = await session.call('get_tour_plan', { placeId: 999_999 }); + expect(run.ok).toBe(false); + expect(String(run.value)).not.toContain('were loaded in this turn'); + } finally { + await session.close(); + } + }); + it('ASTTOOL-014: an undo session holds its target and nothing else', async () => { const { userId, tripId } = await ownerWithTrips(); const session = await tools.open(userId, tripId, new Set(['delete_place']));