Skip to content

Commit 942afc3

Browse files
jonwalstedtclaude
andcommitted
fix(agent-builder): address backend review findings on bulk alerts attachment
- Bound alertIds to [1,20] with Zod .min(1).max(20); empty batches now rejected at validation rather than producing a zero-alert attachment - Add server-side guard in getAlertsById throwing when ids.length > 20, preventing unbounded ES terms queries regardless of call site - Remove try/catch around resolveMaxContentLength so resolution errors propagate visibly instead of silently truncating to the 10k default - Fix agent description tool name: attachments_read → attachment_read, consistent with the framework system prompt - Fix misleading JSDoc on maxContentLength: remove "highest value wins" claim; limit is applied per-attachment, not across types Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent e2fb7eb commit 942afc3

7 files changed

Lines changed: 36 additions & 21 deletions

File tree

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -74,8 +74,8 @@ export interface AttachmentTypeDefinition<TType extends string = string, TConten
7474
*/
7575
isReadonly?: boolean;
7676
/**
77-
* Maximum content length (in characters) allowed when this type is presented inline to the LLM.
78-
* When multiple attachment types are present, the highest value wins.
77+
* Maximum content length (in characters) for attachments of this type when presented inline
78+
* to the LLM. Applied per-attachment — each attachment is truncated to its own type's limit.
7979
* Defaults to the global DEFAULT_MAX_CONTENT_LENGTH (10 000).
8080
*/
8181
maxContentLength?: number;

x-pack/platform/plugins/shared/agent_builder/server/services/execution/run_agent/utils/attachment_presentation.test.ts

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -136,18 +136,17 @@ describe('attachment_presentation', () => {
136136
expect(result.content.length).toBeGreaterThan(500);
137137
});
138138

139-
it('should fall back to default when resolveMaxContentLength throws', async () => {
139+
it('should propagate when resolveMaxContentLength throws', async () => {
140140
const largeContent = 'x'.repeat(15000);
141141
const attachments = [createMockAttachment('1', 'text', largeContent)];
142142

143-
const result = await prepareAttachmentPresentation(attachments, {
144-
resolveMaxContentLength: () => {
145-
throw new Error('resolver error');
146-
},
147-
});
148-
149-
expect(result.content).toContain('[content truncated');
150-
expect(result.content.length).toBeGreaterThan(500);
143+
await expect(
144+
prepareAttachmentPresentation(attachments, {
145+
resolveMaxContentLength: () => {
146+
throw new Error('resolver error');
147+
},
148+
})
149+
).rejects.toThrow('resolver error');
151150
});
152151

153152
it('should handle visualization type as JSON', async () => {

x-pack/platform/plugins/shared/agent_builder/server/services/execution/run_agent/utils/attachment_presentation.ts

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -111,12 +111,7 @@ const formatInlineAttachments = async (
111111
(formatContent ? await formatContent(attachment, latest.data) : undefined) ??
112112
formatAttachmentContent(attachment, latest.data);
113113

114-
let effectiveMax = DEFAULT_MAX_CONTENT_LENGTH;
115-
try {
116-
effectiveMax = resolveMaxContentLength?.(attachment) ?? DEFAULT_MAX_CONTENT_LENGTH;
117-
} catch {
118-
// fall back to default if per-attachment resolution fails
119-
}
114+
const effectiveMax = resolveMaxContentLength?.(attachment) ?? DEFAULT_MAX_CONTENT_LENGTH;
120115
if (contentStr.length > effectiveMax) {
121116
contentStr =
122117
contentStr.substring(0, effectiveMax) +

x-pack/solutions/security/plugins/security_solution/server/agent_builder/attachments/alerts.test.ts

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -63,10 +63,18 @@ describe('createBulkAlertsAttachmentType', () => {
6363
}
6464
});
6565

66-
it('returns valid with an empty alertIds array', async () => {
66+
it('returns invalid with an empty alertIds array', async () => {
6767
const result = await attachmentType.validate({ alertIds: [] });
6868

69-
expect(result.valid).toBe(true);
69+
expect(result.valid).toBe(false);
70+
});
71+
72+
it('returns invalid when alertIds exceeds 20 entries', async () => {
73+
const result = await attachmentType.validate({
74+
alertIds: Array.from({ length: 21 }, (_, i) => `id-${i}`),
75+
});
76+
77+
expect(result.valid).toBe(false);
7078
});
7179

7280
it('returns invalid when alertIds field is missing', async () => {

x-pack/solutions/security/plugins/security_solution/server/agent_builder/attachments/alerts.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ import { getAlertsById } from '../tools/get_alerts_by_id';
2121
import { securityAttachmentDataSchema } from './security_attachment_data_schema';
2222

2323
export const bulkAlertsAttachmentDataSchema = securityAttachmentDataSchema.extend({
24-
alertIds: z.array(z.string()),
24+
alertIds: z.array(z.string()).min(1).max(20),
2525
});
2626

2727
export type BulkAlertsAttachmentData = z.infer<typeof bulkAlertsAttachmentDataSchema>;
@@ -101,7 +101,7 @@ export const createBulkAlertsAttachmentType = (
101101
102102
When the conversation has many batches (summary mode), the system shows metadata only. Read each batch before answering:
103103
- Each batch appears as <attachment attachment_id="<some-id>" type="security.alerts" ...> in the XML
104-
- Call the attachment read tool (attachments_read) and pass the attachment_id value directly as the "attachment_id" parameter
104+
- Call the attachment read tool (attachment_read) and pass the attachment_id value directly as the "attachment_id" parameter
105105
- Read ALL batches before forming conclusions
106106
107107
Process each batch in order:

x-pack/solutions/security/plugins/security_solution/server/agent_builder/tools/get_alerts_by_id.test.ts

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,16 @@ describe('getAlertsById', () => {
2626
expect(esClient.search).not.toHaveBeenCalled();
2727
});
2828

29+
it('throws when more than 20 ids are provided', async () => {
30+
const esClient = makeClient([]);
31+
const ids = Array.from({ length: 21 }, (_, i) => `id-${i}`);
32+
33+
await expect(getAlertsById({ esClient, index, ids })).rejects.toThrow(
34+
'getAlertsById: ids.length (21) exceeds the maximum of 20'
35+
);
36+
expect(esClient.search).not.toHaveBeenCalled();
37+
});
38+
2939
it('returns hits keyed by _id with _source as the value', async () => {
3040
const source1 = { 'kibana.alert.rule.name': 'Rule A' };
3141
const source2 = { 'kibana.alert.rule.name': 'Rule B' };

x-pack/solutions/security/plugins/security_solution/server/agent_builder/tools/get_alerts_by_id.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,9 @@ export const getAlertsById = async ({
2323
if (ids.length === 0) {
2424
return {};
2525
}
26+
if (ids.length > 20) {
27+
throw new Error(`getAlertsById: ids.length (${ids.length}) exceeds the maximum of 20`);
28+
}
2629

2730
const response = await esClient.search({
2831
index,

0 commit comments

Comments
 (0)