From 4fc63fc2a384efb6cf11e8432013b2ef5573955b Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Tue, 28 Jul 2026 15:26:10 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=A4=9D=20fix:=20No=20Labels=20for=20Pure?= =?UTF-8?q?=20Handoff=20Batches?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-sixteen review: a PostToolBatch containing only `transfer_to_*` calls claimed a label slot, but transfer parts are never groupable — the client flushed the handoff card standalone and the label orphaned into a stray line after it, restating what the card already says. Two-sided fix: - Hook (runtime.ts): a batch whose every entry is a transfer call claims nothing — no slot, no model call, no `maxPerRun` consumption. Mixed batches still label (the header describes the real work). - Renderer (groupToolCalls.ts): an orphan label whose `tool_call_ids` are all transfer calls is dropped instead of rendered standalone, covering content persisted before the hook-side skip. The round's two P1s are repeats answered on-thread: the packages/api extraction (maintainer-decided follow-up, recorded in the description) and the sixth restatement of the edited+reconnect index limitation. Tests: transfer-only batch claims nothing, mixed batch still claims (runtime.spec); transfer-only orphan label dropped, real-batch orphan label still renders (groupToolCalls.test). --- .../utils/__tests__/groupToolCalls.test.ts | 38 ++++++++++++++ client/src/utils/groupToolCalls.ts | 29 ++++++++++- .../activityLabels/__tests__/runtime.spec.ts | 51 +++++++++++++++++++ .../api/src/agents/activityLabels/runtime.ts | 12 +++++ 4 files changed, 128 insertions(+), 2 deletions(-) diff --git a/client/src/utils/__tests__/groupToolCalls.test.ts b/client/src/utils/__tests__/groupToolCalls.test.ts index b3f9ed5f01..41244cf733 100644 --- a/client/src/utils/__tests__/groupToolCalls.test.ts +++ b/client/src/utils/__tests__/groupToolCalls.test.ts @@ -94,6 +94,44 @@ describe('groupSequentialToolCalls with activity labels', () => { expect(group.labelPart).toBeUndefined(); }); + /** A pure-handoff batch's label has nothing to head: the transfer card + * names the destination, transfers are never groupable, and the label + * would render as a stray line after the card. */ + it('drops an orphan label whose batch was only transfer calls', () => { + const transfer = { + type: ContentTypes.TOOL_CALL, + [ContentTypes.TOOL_CALL]: { id: 'x1', name: 'lc_transfer_to_billing', args: '{}' }, + } as unknown as TMessageContentParts; + const transferLabel = { + type: ContentTypes.ACTIVITY_LABEL, + [ContentTypes.ACTIVITY_LABEL]: 'Handed off to billing', + tool_call_ids: ['x1'], + pending: false, + } as unknown as TMessageContentParts; + + const grouped = groupSequentialToolCalls(withIndex([transfer, transferLabel])); + + expect(grouped).toHaveLength(1); + expect(grouped[0]).toMatchObject({ type: 'single' }); + expect((grouped[0] as { part: PartWithIndex }).part.idx).toBe(0); + }); + + it('keeps rendering an orphan label whose batch had real tool calls', () => { + const orphanLabel = { + type: ContentTypes.ACTIVITY_LABEL, + [ContentTypes.ACTIVITY_LABEL]: 'Searched the docs', + tool_call_ids: ['t9'], + pending: false, + } as unknown as TMessageContentParts; + + const grouped = groupSequentialToolCalls(withIndex([orphanLabel])); + + expect(grouped).toHaveLength(1); + expect((grouped[0] as { part: PartWithIndex }).part.part).toMatchObject({ + [ContentTypes.ACTIVITY_LABEL]: 'Searched the docs', + }); + }); + /** A filled label claims only its own batch — never one behind a blank slot. */ it('stops a filled label from claiming a batch behind a blank label', () => { const grouped = groupSequentialToolCalls( diff --git a/client/src/utils/groupToolCalls.ts b/client/src/utils/groupToolCalls.ts index a6370f4be6..9edd9511ca 100644 --- a/client/src/utils/groupToolCalls.ts +++ b/client/src/utils/groupToolCalls.ts @@ -23,6 +23,29 @@ function isGroupableToolCall(part: TMessageContentParts): boolean { return true; } +/** + * True when every tool call the label covers is a `transfer_to_*` handoff. + * Transfer parts are never groupable, so such a label can only orphan into a + * stray line after the handoff card — which already names the destination. + * Content persisted before the server stopped claiming labels for pure + * handoff batches still carries these; they are dropped at render. + */ +function isTransferOnlyLabel(labelPart: TMessageContentParts, allParts: PartWithIndex[]): boolean { + const ids = (labelPart as { tool_call_ids?: unknown }).tool_call_ids; + if (!Array.isArray(ids) || ids.length === 0) { + return false; + } + return ids.every((id) => + allParts.some(({ part }) => { + if (part?.type !== ContentTypes.TOOL_CALL) { + return false; + } + const toolCall = part[ContentTypes.TOOL_CALL] as Agents.ToolCall | undefined; + return toolCall?.id === id && toolCall?.name?.startsWith(Constants.LC_TRANSFER_TO_) === true; + }), + ); +} + /** * Groups message content for rendering. * @@ -96,8 +119,10 @@ export function groupSequentialToolCalls(parts: PartWithIndex[]): GroupedPart[] flushWithoutLabel(); if (claimed.length > 0) { result.push({ type: 'tool-group', parts: claimed, labelPart: item }); - } else { - /** Orphan label (block parts hidden/filtered): renders standalone. */ + } else if (!isTransferOnlyLabel(item.part, parts)) { + /** Orphan label (block parts hidden/filtered): renders standalone — + * UNLESS its batch was only handoff calls, where the transfer card + * already says everything and the label would be a stray line. */ result.push({ type: 'single', part: item }); } continue; diff --git a/packages/api/src/agents/activityLabels/__tests__/runtime.spec.ts b/packages/api/src/agents/activityLabels/__tests__/runtime.spec.ts index c99a6b0440..59ecdae66d 100644 --- a/packages/api/src/agents/activityLabels/__tests__/runtime.spec.ts +++ b/packages/api/src/agents/activityLabels/__tests__/runtime.spec.ts @@ -251,6 +251,57 @@ describe('createActivityLabelHook', () => { expect(slots[0].filled).toEqual([null]); }); + /** A pure-handoff batch gets no label: the transfer card already names + * the destination, and the client renders transfers standalone, so a + * label could only orphan. Must not consume `maxPerRun` either. */ + it('claims nothing for a batch of only transfer calls', async () => { + const hook = createActivityLabelHook({ claimSlot, resolveLLM }); + await hook( + batchInput({ + entries: [ + { + toolName: 'lc_transfer_to_billing_agent', + toolInput: {}, + toolUseId: 'x1', + status: 'success', + toolOutput: '', + }, + ], + }), + new AbortController().signal, + ); + await flushDetached(); + expect(slots).toHaveLength(0); + expect(mockInvoke).not.toHaveBeenCalled(); + }); + + it('still claims for a mixed batch containing a transfer call', async () => { + const hook = createActivityLabelHook({ claimSlot, resolveLLM }); + await hook( + batchInput({ + entries: [ + { + toolName: 'lc_transfer_to_billing_agent', + toolInput: {}, + toolUseId: 'x1', + status: 'success', + toolOutput: '', + }, + { + toolName: 'web_search', + toolInput: { query: 'refund policy' }, + toolUseId: 'x2', + status: 'success', + toolOutput: 'found it', + }, + ], + }), + new AbortController().signal, + ); + await flushDetached(); + expect(slots).toHaveLength(1); + }); + it('skips subagent scopes entirely', async () => { const hook = createActivityLabelHook({ claimSlot, resolveLLM }); await hook(batchInput({ agentId: 'subagent-1' }), new AbortController().signal); diff --git a/packages/api/src/agents/activityLabels/runtime.ts b/packages/api/src/agents/activityLabels/runtime.ts index 8e4c069f8f..53a90d1073 100644 --- a/packages/api/src/agents/activityLabels/runtime.ts +++ b/packages/api/src/agents/activityLabels/runtime.ts @@ -1,4 +1,5 @@ import { logger } from '@librechat/data-schemas'; +import { Constants } from 'librechat-data-provider'; import { initializeModel } from '@librechat/agents'; import type { ClientOptions, HookCallback, HookInputByEvent, Providers } from '@librechat/agents'; @@ -429,6 +430,17 @@ export function createActivityLabelHook( ) { return {}; } + /** A batch that is ONLY handoff calls gets no label. The transfer card + * already names the destination, the client renders transfer parts + * standalone (never groupable), so a claimed label could only orphan + * into a stray line after the card — and the model call would be spent + * describing what the card already says. Skipped BEFORE the quota so + * handoffs never consume `maxPerRun`. */ + if ( + input.entries.every((entry) => entry.toolName?.startsWith(Constants.LC_TRANSFER_TO_) === true) + ) { + return {}; + } generated += 1; const slot = opts.claimSlot({ ...classifyBatch(input.entries),