Skip to content

Commit 0602f08

Browse files
jonwalstedtclaude
andcommitted
fix(agent-builder): deduplicate grouped attachment refs in RoundAttachmentReferences
When bulk-adding alerts the chat route chunked them into multiple AttachmentInput batches, causing one "Attachment added:" line per batch instead of one per logical group. - Add group_id and description to Attachment/VersionedAttachment/AttachmentInput types and propagate through server validation, state manager, and chat route - Stamp group_id and description on group items at the flattenAttachments boundary (the sole serialisation point), not at call sites - Deduplicate by group_id in RoundAttachmentReferences; actor-filter runs before the seenGroupIds slot is consumed so a non-matching actor ref cannot silently block a matching one from rendering - Fix description falsy-check in AttachmentStateManager (use !== undefined) - Add tests for RoundAttachmentReferences dedup logic (incl. actor-filter regression), flattenAttachments group stamping, and buildOptimisticAttachments group_id/description passthrough Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent 18b1e81 commit 0602f08

13 files changed

Lines changed: 312 additions & 7 deletions

File tree

x-pack/platform/packages/shared/agent-builder/agent-builder-common/attachments/attachments.ts

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,8 @@ export interface Attachment<
2020
type: Type;
2121
/** data bound to the attachment */
2222
data: DataType;
23+
/** Human-readable description of the attachment */
24+
description?: string;
2325
/** should the attachment be hidden from the user - e.g. for screen context */
2426
hidden?: boolean;
2527
/**
@@ -28,6 +30,13 @@ export interface Attachment<
2830
* Undefined for by-value attachments.
2931
*/
3032
origin?: string;
33+
/**
34+
* Stable identifier for the logical group this attachment belongs to.
35+
* Attachments sharing the same group_id were submitted together as a single
36+
* logical entity (e.g. multiple alert batches from one bulk-add action).
37+
* Undefined for standalone attachments.
38+
*/
39+
group_id?: string;
3140
}
3241

3342
/**

x-pack/platform/packages/shared/agent-builder/agent-builder-common/attachments/versioned_attachment.ts

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,13 @@ export interface VersionedAttachment<
5050
readonly?: boolean;
5151
/** The client-provided ID if this attachment was created with one (e.g., via flyout configuration) */
5252
client_id?: string;
53+
/**
54+
* Stable identifier for the logical group this attachment belongs to.
55+
* Attachments sharing the same group_id were submitted together as a single
56+
* logical entity (e.g. multiple alert batches from one bulk-add action).
57+
* Undefined for standalone attachments.
58+
*/
59+
group_id?: string;
5360
/**
5461
* Origin/reference info for attachments created from external sources.
5562
* For saved-object-backed types this is the saved object ID.
@@ -153,6 +160,16 @@ export interface AttachmentInput<
153160
hidden?: boolean;
154161
/** Whether the attachment should be read-only */
155162
readonly?: boolean;
163+
/**
164+
* Stable identifier for the logical group this attachment belongs to.
165+
* Attachments sharing the same group_id were submitted together as a single
166+
* logical entity (e.g. multiple alert batches from one bulk-add action).
167+
* Undefined for standalone attachments.
168+
*
169+
* When this input is part of an AttachmentGroup, flattenAttachments always
170+
* stamps this field with the group's id, overriding any value set here.
171+
*/
172+
group_id?: string;
156173
}
157174

158175
// Zod schemas for validation
@@ -198,6 +215,7 @@ export const versionedAttachmentSchema = z.object({
198215
client_id: z.string().optional(),
199216
origin: z.string().optional(),
200217
origin_snapshot_at: z.string().optional(),
218+
group_id: z.string().optional(),
201219
});
202220

203221
export const attachmentInputSchema = z.object({
@@ -208,6 +226,7 @@ export const attachmentInputSchema = z.object({
208226
description: z.string().optional(),
209227
hidden: z.boolean().optional(),
210228
readonly: z.boolean().optional(),
229+
group_id: z.string().optional(),
211230
});
212231

213232
/**

x-pack/platform/packages/shared/agent-builder/agent-builder-server/attachments/attachment_state_manager.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -349,13 +349,14 @@ class AttachmentStateManagerImpl implements AttachmentStateManager {
349349
versions: [version],
350350
current_version: 1,
351351
active: true,
352-
...(input.description && { description: input.description }),
352+
...(input.description !== undefined && { description: input.description }),
353353
...(input.hidden !== undefined && { hidden: input.hidden }),
354354
readonly: input.readonly ?? this.getDefaultReadonly(input.type),
355355
...(input.origin !== undefined && { origin: input.origin }),
356356
// When created with origin (by-reference), record snapshot time for isStale comparison.
357357
// By-value attachments leave this undefined.
358358
...(input.origin !== undefined && { origin_snapshot_at: now }),
359+
...(input.group_id !== undefined && { group_id: input.group_id }),
359360
};
360361

361362
this.attachments.set(id, attachment);
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,168 @@
1+
/*
2+
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
3+
* or more contributor license agreements. Licensed under the Elastic License
4+
* 2.0; you may not use this file except in compliance with the Elastic License
5+
* 2.0.
6+
*/
7+
8+
import React from 'react';
9+
import { render, screen } from '@testing-library/react';
10+
import type {
11+
VersionedAttachment,
12+
Attachment,
13+
AttachmentVersionRef,
14+
} from '@kbn/agent-builder-common/attachments';
15+
import {
16+
ATTACHMENT_REF_ACTOR,
17+
ATTACHMENT_REF_OPERATION,
18+
} from '@kbn/agent-builder-common/attachments';
19+
import { RoundAttachmentReferences } from './round_attachment_references';
20+
21+
const makeVersioned = (
22+
id: string,
23+
description?: string,
24+
group_id?: string
25+
): VersionedAttachment => ({
26+
id,
27+
type: 'text',
28+
versions: [{ version: 1, data: {}, created_at: '2024-01-01T00:00:00Z', content_hash: 'x' }],
29+
current_version: 1,
30+
active: true,
31+
...(description !== undefined ? { description } : {}),
32+
...(group_id !== undefined ? { group_id } : {}),
33+
});
34+
35+
const makeRef = (
36+
id: string,
37+
actor: (typeof ATTACHMENT_REF_ACTOR)[keyof typeof ATTACHMENT_REF_ACTOR] = ATTACHMENT_REF_ACTOR.user,
38+
operation?: (typeof ATTACHMENT_REF_OPERATION)[keyof typeof ATTACHMENT_REF_OPERATION]
39+
): AttachmentVersionRef => ({
40+
attachment_id: id,
41+
version: 1,
42+
actor,
43+
operation,
44+
});
45+
46+
const makeFallback = (id: string, description?: string, group_id?: string): Attachment => ({
47+
id,
48+
type: 'text',
49+
data: {},
50+
...(description !== undefined ? { description } : {}),
51+
...(group_id !== undefined ? { group_id } : {}),
52+
});
53+
54+
describe('RoundAttachmentReferences', () => {
55+
it('renders nothing when there are no refs or attachments', () => {
56+
const { container } = render(<RoundAttachmentReferences />);
57+
expect(container.firstChild).toBeNull();
58+
});
59+
60+
it('renders nothing when refs are empty', () => {
61+
const { container } = render(
62+
<RoundAttachmentReferences
63+
attachmentRefs={[]}
64+
conversationAttachments={[makeVersioned('a', 'Label A')]}
65+
/>
66+
);
67+
expect(container.firstChild).toBeNull();
68+
});
69+
70+
it('renders one line per resolved attachment', () => {
71+
render(
72+
<RoundAttachmentReferences
73+
attachmentRefs={[makeRef('a'), makeRef('b')]}
74+
conversationAttachments={[makeVersioned('a', 'First'), makeVersioned('b', 'Second')]}
75+
/>
76+
);
77+
78+
expect(screen.getByText(/First/)).toBeInTheDocument();
79+
expect(screen.getByText(/Second/)).toBeInTheDocument();
80+
});
81+
82+
it('deduplicates by group_id — renders only one line for a group', () => {
83+
render(
84+
<RoundAttachmentReferences
85+
attachmentRefs={[makeRef('a1'), makeRef('a2')]}
86+
conversationAttachments={[
87+
makeVersioned('a1', '27 Alerts', 'group-1'),
88+
makeVersioned('a2', '27 Alerts', 'group-1'),
89+
]}
90+
/>
91+
);
92+
93+
const lines = screen.getAllByText(/27 Alerts/);
94+
expect(lines).toHaveLength(1);
95+
});
96+
97+
it('actor filter applied before group dedup — matching actor ref renders even when preceded by non-matching actor ref for the same group', () => {
98+
// Regression for: system ref consumed group slot before actor filter, so user ref was never rendered.
99+
render(
100+
<RoundAttachmentReferences
101+
attachmentRefs={[
102+
makeRef('a1', ATTACHMENT_REF_ACTOR.system), // non-matching — must NOT consume slot
103+
makeRef('a2', ATTACHMENT_REF_ACTOR.user), // matching — must render
104+
]}
105+
conversationAttachments={[
106+
makeVersioned('a1', '27 Alerts', 'group-1'),
107+
makeVersioned('a2', '27 Alerts', 'group-1'),
108+
]}
109+
actorFilter={[ATTACHMENT_REF_ACTOR.user]}
110+
/>
111+
);
112+
113+
expect(screen.getByText(/27 Alerts/)).toBeInTheDocument();
114+
});
115+
116+
it('renders nothing when all refs are filtered out by actorFilter', () => {
117+
const { container } = render(
118+
<RoundAttachmentReferences
119+
attachmentRefs={[makeRef('a', ATTACHMENT_REF_ACTOR.system)]}
120+
conversationAttachments={[makeVersioned('a', 'Label')]}
121+
actorFilter={[ATTACHMENT_REF_ACTOR.user]}
122+
/>
123+
);
124+
expect(container.firstChild).toBeNull();
125+
});
126+
127+
it('skips hidden attachments', () => {
128+
const hidden: VersionedAttachment = { ...makeVersioned('h', 'Hidden Label'), hidden: true };
129+
const { container } = render(
130+
<RoundAttachmentReferences
131+
attachmentRefs={[makeRef('h')]}
132+
conversationAttachments={[hidden]}
133+
/>
134+
);
135+
expect(container.firstChild).toBeNull();
136+
});
137+
138+
it('skips refs with operation=read', () => {
139+
const { container } = render(
140+
<RoundAttachmentReferences
141+
attachmentRefs={[makeRef('a', ATTACHMENT_REF_ACTOR.user, ATTACHMENT_REF_OPERATION.read)]}
142+
conversationAttachments={[makeVersioned('a', 'Label')]}
143+
/>
144+
);
145+
expect(container.firstChild).toBeNull();
146+
});
147+
148+
it('uses fallbackAttachments when no attachmentRefs are provided', () => {
149+
render(
150+
<RoundAttachmentReferences fallbackAttachments={[makeFallback('f1', 'Fallback Label')]} />
151+
);
152+
expect(screen.getByText(/Fallback Label/)).toBeInTheDocument();
153+
});
154+
155+
it('deduplicates fallback attachments by group_id', () => {
156+
render(
157+
<RoundAttachmentReferences
158+
fallbackAttachments={[
159+
makeFallback('f1', '27 Alerts', 'group-1'),
160+
makeFallback('f2', '27 Alerts', 'group-1'),
161+
]}
162+
/>
163+
);
164+
165+
const lines = screen.getAllByText(/27 Alerts/);
166+
expect(lines).toHaveLength(1);
167+
});
168+
});

x-pack/platform/plugins/shared/agent_builder/public/application/components/conversations/conversation_rounds/round_attachment_references.tsx

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,8 @@ const buildFallbackVersionedAttachments = (attachments: Attachment[]): Versioned
8888
current_version: 1,
8989
active: true,
9090
hidden: attachment.hidden,
91+
...(attachment.group_id !== undefined ? { group_id: attachment.group_id } : {}),
92+
...(attachment.description !== undefined ? { description: attachment.description } : {}),
9193
}));
9294
};
9395

@@ -134,6 +136,7 @@ export const RoundAttachmentReferences: React.FC<RoundAttachmentReferencesProps>
134136
}
135137

136138
const resolved: ResolvedReference[] = [];
139+
const seenGroupIds = new Set<string>();
137140
for (const ref of refs) {
138141
const attachment = attachmentMap.get(ref.attachment_id);
139142
if (!attachment) {
@@ -150,6 +153,13 @@ export const RoundAttachmentReferences: React.FC<RoundAttachmentReferencesProps>
150153
continue;
151154
}
152155

156+
if (attachment.group_id) {
157+
if (seenGroupIds.has(attachment.group_id)) {
158+
continue;
159+
}
160+
seenGroupIds.add(attachment.group_id);
161+
}
162+
153163
resolved.push({
154164
attachment,
155165
version: ref.version,

x-pack/platform/plugins/shared/agent_builder/public/application/context/conversation/flatten_attachments.test.ts

Lines changed: 39 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,11 @@ describe('flattenAttachments', () => {
3636
const item1 = input('security.alerts', 'i1');
3737
const item2 = input('security.alerts', 'i2');
3838
const g = group('g1', [item1, item2]);
39-
expect(flattenAttachments([g])).toEqual([item1, item2]);
39+
const result = flattenAttachments([g]);
40+
expect(result).toEqual([
41+
{ ...item1, group_id: 'g1', description: '2 Alerts' },
42+
{ ...item2, group_id: 'g1', description: '2 Alerts' },
43+
]);
4044
});
4145

4246
it('flattens a mixed list — groups expand in place, individuals pass through', () => {
@@ -45,7 +49,11 @@ describe('flattenAttachments', () => {
4549
const item2 = input('security.alerts', 'a2');
4650
const g = group('g1', [item1, item2]);
4751
const result = flattenAttachments([solo, g]);
48-
expect(result).toEqual([solo, item1, item2]);
52+
expect(result).toEqual([
53+
solo,
54+
{ ...item1, group_id: 'g1', description: '2 Alerts' },
55+
{ ...item2, group_id: 'g1', description: '2 Alerts' },
56+
]);
4957
});
5058

5159
it('preserves order across multiple groups and individuals', () => {
@@ -59,7 +67,13 @@ describe('flattenAttachments', () => {
5967
b,
6068
group('g2', g2Items),
6169
];
62-
expect(flattenAttachments(attachments)).toEqual([a, ...g1Items, b, ...g2Items]);
70+
expect(flattenAttachments(attachments)).toEqual([
71+
a,
72+
{ ...g1Items[0], group_id: 'g1', description: '2 Alerts' },
73+
{ ...g1Items[1], group_id: 'g1', description: '2 Alerts' },
74+
b,
75+
{ ...g2Items[0], group_id: 'g2', description: '1 Alerts' },
76+
]);
6377
});
6478

6579
it('handles a group with no items', () => {
@@ -73,4 +87,26 @@ describe('flattenAttachments', () => {
7387
flattenAttachments(items);
7488
expect(JSON.stringify(items)).toBe(snapshot);
7589
});
90+
91+
it('does not stamp group_id or description on individual attachments', () => {
92+
const a = input('text', 'a');
93+
const result = flattenAttachments([a]);
94+
95+
expect(result[0]).not.toHaveProperty('group_id');
96+
expect(result[0]).not.toHaveProperty('description');
97+
});
98+
99+
it('preserves an item description when it already has one, does not overwrite with group label', () => {
100+
const item: AttachmentInput = {
101+
type: 'security.alerts',
102+
id: 'i1',
103+
data: {},
104+
description: 'custom',
105+
};
106+
const g = group('g1', [item]);
107+
const result = flattenAttachments([g]);
108+
109+
expect(result[0].description).toBe('custom');
110+
expect(result[0].group_id).toBe('g1');
111+
});
76112
});

x-pack/platform/plugins/shared/agent_builder/public/application/context/conversation/flatten_attachments.ts

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,19 @@ import { isAttachmentGroup } from '@kbn/agent-builder-common/attachments';
1515
* Flattens ConversationAttachment[] to AttachmentInput[] for serialization.
1616
* AttachmentGroups are expanded to their constituent items; individual attachments pass through unchanged.
1717
* This is the only place groups are dissolved — the server always receives AttachmentInput[].
18+
*
19+
* Group items are stamped with group_id and description at this boundary:
20+
* - group_id is always set to the group's id, overriding any pre-existing value on the item.
21+
* An item pre-setting group_id would be a caller error — group identity belongs to the group.
22+
* - description falls back to the group's label if the item does not supply its own.
1823
*/
1924
export const flattenAttachments = (attachments: ConversationAttachment[]): AttachmentInput[] =>
20-
attachments.flatMap((a) => (isAttachmentGroup(a) ? a.items : [a]));
25+
attachments.flatMap((a) =>
26+
isAttachmentGroup(a)
27+
? a.items.map((item) => ({
28+
...item,
29+
group_id: a.id,
30+
description: item.description ?? a.label,
31+
}))
32+
: [a]
33+
);

0 commit comments

Comments
 (0)