From ab40e821237e8b8dc998dd2c9dc3441c18357bc1 Mon Sep 17 00:00:00 2001 From: agent_coder Date: Fri, 10 Jul 2026 00:58:55 +0300 Subject: [PATCH] =?UTF-8?q?fix(tools):=20=D0=BA=D0=BE=D0=BC=D0=BC=D0=B5?= =?UTF-8?q?=D0=BD=D1=82=D0=B0=D1=80=D0=B8=D0=B9=D0=BD=D0=B0=D1=8F=20=D0=BE?= =?UTF-8?q?=D0=B1=D1=91=D1=80=D1=82=D0=BA=D0=B0=20=D0=9A=D0=9E=D0=9C=D0=9F?= =?UTF-8?q?=D0=9E=D0=9D=D0=A3=D0=95=D0=A2=20=D1=81=D0=BE=D0=B1=D1=81=D1=82?= =?UTF-8?q?=D0=B2=D0=B5=D0=BD=D0=BD=D1=8B=D0=B9=20toModelOutput=20=D1=82?= =?UTF-8?q?=D1=83=D0=BB=D0=B0,=20=D0=B0=20=D0=BD=D0=B5=20=D0=B7=D0=B0?= =?UTF-8?q?=D1=82=D0=B8=D1=80=D0=B0=D0=B5=D1=82=20(=D1=80=D0=B5=D0=B2?= =?UTF-8?q?=D1=8C=D1=8E=20#428)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit wrapToolsWithCommentSignal всегда ставил свой toModelOutput, молча выбрасывая собственный toModelOutput инструмента (латентная ловушка — будущий тул со своим toModelOutput тихо сломался бы). Теперь база = origToModelOutput(info) при наличии, иначе воспроизведённый дефолт SDK; no-signal путь возвращает базу дословно, signal- путь = части базы (modelOutputToParts: text/json/content) + элемент сигнала последним. execute по-прежнему возвращает СЫРОЙ результат -> part.output/цитаты байт-идентичны. Дефолтный путь (единственный исполняемый сегодня) байт-идентичен и SDK-дефолту, и до-фиксовому signal-пути (проверено повторным ревью). json-ветку загардил ?? null для симметрии с fallback. +2 теста: тул со своим text/content toModelOutput — база честно сохраняется и в no-signal, и в signal (сигнал добавлен последним). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../ai-chat/tools/ai-chat-tools.service.ts | 62 ++++++++++++++-- .../tools/comment-signal-inapp.spec.ts | 74 +++++++++++++++++++ 2 files changed, 128 insertions(+), 8 deletions(-) diff --git a/apps/server/src/core/ai-chat/tools/ai-chat-tools.service.ts b/apps/server/src/core/ai-chat/tools/ai-chat-tools.service.ts index 07ce76d4..05e75b3b 100644 --- a/apps/server/src/core/ai-chat/tools/ai-chat-tools.service.ts +++ b/apps/server/src/core/ai-chat/tools/ai-chat-tools.service.ts @@ -881,6 +881,13 @@ export class AiChatToolsService { * A per-`toolCallId` map bridges `execute` -> `toModelOutput` (both receive the * toolCallId), so parallel tool calls never cross-talk. Exported for unit * testing without a live model/transport. + * + * NOTE for future tool authors: this wrapper OWNS `toModelOutput` on every + * wrapped tool, but it COMPOSES rather than discards a tool's OWN + * `toModelOutput`. If a tool defines one, it is used as the base model output + * (honored verbatim on the no-signal path; flattened and kept, with the signal + * appended, on the signal path). A custom `toModelOutput` is therefore never + * silently dropped. */ export function wrapToolsWithCommentSignal( tools: Record, @@ -901,8 +908,39 @@ export function wrapToolsWithCommentSignal( ? { type: 'text' as const, value: output } : { type: 'json' as const, value: (output ?? null) as unknown }; + // Flatten a BASE model-output (the tool's OWN toModelOutput result, or the SDK + // default) into SDK `content` parts, so the passive signal can be appended as a + // trailing text element WITHOUT discarding the base. Covers the three real SDK + // shapes (text/json/content); falls back defensively for anything else. Every + // returned item is a valid SDK content item (text, or a file part spread from + // an existing `content` base). + const modelOutputToParts = (base: unknown, rawOutput: unknown): unknown[] => { + const b = base as { type?: string; value?: unknown }; + if (b?.type === 'text') { + return [{ type: 'text' as const, text: b.value as string }]; + } + if (b?.type === 'json') { + // `?? null` keeps this symmetric with the fallback branch below: a tool that + // (invalidly) returns {type:'json', value:undefined} would otherwise yield a + // non-string text. No current tool defines toModelOutput, so this is defensive. + return [{ type: 'text' as const, text: JSON.stringify(b.value ?? null) }]; + } + if (b?.type === 'content' && Array.isArray(b.value)) { + return [...b.value]; + } + return [ + { type: 'text' as const, text: JSON.stringify(b?.value ?? rawOutput ?? null) }, + ]; + }; + for (const [name, toolDef] of Object.entries(tools)) { const originalExecute = toolDef.execute; + // Capture the tool's OWN toModelOutput (if any) BEFORE we install ours. The + // comment-signal wrapper OWNS `toModelOutput` on the wrapped tool, but it + // COMPOSES rather than discards a tool-defined one: the base model output is + // computed from `origToModelOutput` when present (see below), so a future + // tool that ships its own `toModelOutput` is honored, not silently dropped. + const origToModelOutput = toolDef.toModelOutput; if (typeof originalExecute !== 'function') { wrapped[name] = toolDef; continue; @@ -946,7 +984,9 @@ export function wrapToolsWithCommentSignal( return result; }) as Tool['execute'], // Model-only delivery: append the signal as a SEPARATE content element, - // leaving the streamed/persisted `output` untouched (mirrors MCP). + // leaving the streamed/persisted `output` untouched (mirrors MCP). This + // OWNS toModelOutput but COMPOSES the tool's own (origToModelOutput) into + // the base, so a custom toModelOutput is honored on BOTH paths. toModelOutput: ((info: { toolCallId?: string; input?: unknown; @@ -960,16 +1000,22 @@ export function wrapToolsWithCommentSignal( if (typeof toolCallId === 'string' && line !== undefined) { pendingSignals.delete(toolCallId); } - if (!line) return defaultModelOutput(output); - // Signal present: the raw result as one text element + the signal as a - // second — the model sees BOTH, and the model must NOT dig under a - // `.result` wrapper (the raw shape is unchanged). - const base = - typeof output === 'string' ? output : JSON.stringify(output ?? null); + // BASE = the authoritative model-facing representation of THIS tool's + // result: the tool's own toModelOutput when it defined one, else the + // reproduced SDK default (string -> text, else json). + const base = origToModelOutput + ? (origToModelOutput as (i: unknown) => unknown)(info) + : defaultModelOutput(output); + // No signal: return the BASE unchanged — byte-identical to what the SDK + // (or the tool's own toModelOutput) would have produced. + if (!line) return base; + // Signal present: flatten BASE into content parts, then append the + // signal as a trailing text element — the model sees BOTH the tool's own + // model output AND the signal, with no `.result` wrapper to dig under. return { type: 'content' as const, value: [ - { type: 'text' as const, text: base }, + ...modelOutputToParts(base, output), { type: 'text' as const, text: line }, ], }; diff --git a/apps/server/src/core/ai-chat/tools/comment-signal-inapp.spec.ts b/apps/server/src/core/ai-chat/tools/comment-signal-inapp.spec.ts index 63b9ba26..32d28433 100644 --- a/apps/server/src/core/ai-chat/tools/comment-signal-inapp.spec.ts +++ b/apps/server/src/core/ai-chat/tools/comment-signal-inapp.spec.ts @@ -182,6 +182,80 @@ describe('wrapToolsWithCommentSignal (in-app non-destructive delivery)', () => { expect(signalLineOf(searchModel)).toBe(line); expect(signalLineOf(createModel)).toBe(line); }); + + it("COMPOSES a tool's OWN toModelOutput (text base): no-signal honors it verbatim; signal appends", async () => { + const original = { raw: 'data' }; + // A tool that ships a CUSTOM toModelOutput (a text shape, not the SDK json + // default). The wrapper must honor it, not overwrite it with json(output). + const custom: Tool = { + description: 'x', + inputSchema: {}, + execute: async () => original, + toModelOutput: () => ({ type: 'text' as const, value: 'CUSTOM' }), + } as unknown as Tool; + + // No-signal path: the wrapper returns the tool's own base verbatim. + const noSig = wrapToolsWithCommentSignal({ getPage: custom }, fakeTracker(null)); + const { output: o1, model: m1 } = await run(noSig.getPage, { pageId: 'p1' }); + expect(o1).toBe(original); // part.output still RAW execute result + expect(m1).toEqual({ type: 'text', value: 'CUSTOM' }); + + // Signal path: the base parts are preserved AND the signal is appended, in + // order — both present. + const line = + '[signal] new comments: 4 on page p1 — call listComments(pageId) for details'; + const sig = wrapToolsWithCommentSignal({ getPage: custom }, fakeTracker(line)); + const { output: o2, model: m2 } = await run(sig.getPage, { pageId: 'p1' }); + expect(o2).toBe(original); // part.output unchanged by the signal + const mm = m2 as { type: string; value: Array<{ type: string; text: string }> }; + expect(mm.type).toBe('content'); + expect(mm.value[0]).toEqual({ type: 'text', text: 'CUSTOM' }); // base kept + expect(mm.value[mm.value.length - 1]).toEqual({ type: 'text', text: line }); + expect(mm.value).toHaveLength(2); + }); + + it("COMPOSES a tool's OWN toModelOutput (content base): base parts survive, signal appended after", async () => { + const original = { raw: 'data' }; + // A custom toModelOutput already returning a multi-part `content` shape. + const custom: Tool = { + description: 'x', + inputSchema: {}, + execute: async () => original, + toModelOutput: () => ({ + type: 'content' as const, + value: [ + { type: 'text' as const, text: 'part-A' }, + { type: 'text' as const, text: 'part-B' }, + ], + }), + } as unknown as Tool; + + // No-signal path: content base returned verbatim. + const noSig = wrapToolsWithCommentSignal({ getPage: custom }, fakeTracker(null)); + const { model: m1 } = await run(noSig.getPage, { pageId: 'p1' }); + expect(m1).toEqual({ + type: 'content', + value: [ + { type: 'text', text: 'part-A' }, + { type: 'text', text: 'part-B' }, + ], + }); + + // Signal path: both original parts survive (spread), signal appended last. + const line = + '[signal] new comments: 1 on page p1 — call listComments(pageId) for details'; + const sig = wrapToolsWithCommentSignal({ getPage: custom }, fakeTracker(line)); + const { output, model: m2 } = await run(sig.getPage, { pageId: 'p1' }); + expect(output).toBe(original); + expect(m2).toEqual({ + type: 'content', + value: [ + { type: 'text', text: 'part-A' }, + { type: 'text', text: 'part-B' }, + { type: 'text', text: line }, + ], + }); + }); }); describe('AiChatToolsService forUser + comment signal (real tracker)', () => {