mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-08-04 14:57:42 +00:00
🤝 fix: No Labels for Pure Handoff Batches
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).
This commit is contained in:
parent
1fbc1d405b
commit
4fc63fc2a3
4 changed files with 128 additions and 2 deletions
|
|
@ -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(
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
|
|
|
|||
|
|
@ -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),
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue