Skip to content

Commit 83af8c3

Browse files
authored
fix(server): make push notification config id optional with server-side UUID (#543)
# Description ## What `createTaskPushNotificationConfig` accepts id-less requests across all transports: - Handler generates a server-side UUID when `params.id` is empty - Handler returns the persisted record (not input params) - Store no longer falls back to `id = taskId` (caused silent overwrites) - REST transport no longer rejects id-less requests with `RequestMalformedError('id is required')` ## Why Spec §3.1.7: "Created configuration with assigned ID" — the id is the result of the operation, not an input requirement. The `id ||= taskId` fallback meant every parameter-less Create became a destructive upsert on the same row. REST additionally diverged from JSON-RPC by requiring client-supplied id, breaking functional equivalence (§5.1) and rejecting requests from a2a-python / a2a-go clients. Both SDKs accept id-less requests (a2a-python `rest_dispatcher.py:290-307`, a2a-go `rest.go:370-397` + UUIDv7 in `a2asrv/push/store.go:45-48`). Fixes #541 🦕
1 parent cd8f8fc commit 83af8c3

7 files changed

Lines changed: 312 additions & 18 deletions

File tree

src/server/push_notification/push_notification_store.ts

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import { v4 as uuidv4 } from 'uuid';
12
import { TaskPushNotificationConfig } from '../../index.js';
23
import { A2A_LEGACY_PROTOCOL_VERSION } from '../../constants.js';
34
import { ServerCallContext } from '../context.js';
@@ -34,6 +35,12 @@ export interface StoredPushNotificationConfig {
3435
* for push notification configuration operations.
3536
*/
3637
export interface PushNotificationStore {
38+
/**
39+
* Implementations MUST assign a non-empty `pushNotificationConfig.id`
40+
* in place when the caller passes an empty one (spec §3.1.7 — id is
41+
* the *result* of Create). Callers observe the assignment via the same
42+
* reference they passed in.
43+
*/
3744
save(
3845
taskId: string,
3946
context: ServerCallContext,
@@ -102,9 +109,11 @@ export class InMemoryPushNotificationStore implements PushNotificationStore {
102109
const bucket = this._scopedStore.getOrCreateBucket(context);
103110
const entries = bucket.get(taskId) || [];
104111

105-
// Set ID if it's not already set
112+
// Spec §3.1.7 / §5.1: id is the *result* of Create, not an input
113+
// requirement — id-less Creates must produce distinct records, not
114+
// silently upsert onto the same row.
106115
if (!pushNotificationConfig.id) {
107-
pushNotificationConfig.id = taskId;
116+
pushNotificationConfig.id = uuidv4();
108117
}
109118

110119
// Capture the wire version from the request context. ServerCallContext

src/server/request_handler/default_request_handler.ts

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -986,8 +986,7 @@ export class DefaultRequestHandler implements A2ARequestHandler {
986986
}
987987

988988
await this.pushNotificationStore?.save(taskId, context, params);
989-
990-
return params;
989+
return structuredClone(params);
991990
}
992991

993992
async getTaskPushNotificationConfig(

src/server/transports/rest/rest_transport_handler.ts

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -277,9 +277,6 @@ export class RestTransportHandler {
277277
context: ServerCallContext
278278
): Promise<TaskPushNotificationConfig> {
279279
await this.requireCapability('pushNotifications');
280-
if (!config.id) {
281-
throw new RequestMalformedError('id is required');
282-
}
283280
return this.requestHandler.createTaskPushNotificationConfig(config, context);
284281
}
285282

test/server/express/rest_handler.spec.ts

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -612,6 +612,42 @@ describe('restHandler', () => {
612612
})
613613
.expect(400);
614614
});
615+
616+
it('should accept an id-less body and return 201 with the server-assigned id', async () => {
617+
// Spec §3.1.7 / §5.1: id is optional across all transports — the
618+
// handler generates a UUID server-side. REST previously rejected
619+
// id-less requests with `RequestMalformedError('id is required')`,
620+
// breaking parity with JSON-RPC, gRPC, and the reference
621+
// a2a-python / a2a-go REST dispatchers (which both accept id-less
622+
// bodies and return the persisted record).
623+
const assignedConfig: TaskPushNotificationConfig = {
624+
...mockConfig,
625+
id: 'server-assigned-uuid',
626+
};
627+
(mockRequestHandler.createTaskPushNotificationConfig as Mock).mockResolvedValue(
628+
assignedConfig
629+
);
630+
631+
const response = await request(app)
632+
.post('/tasks/task-1/pushNotificationConfigs')
633+
.set('A2A-Version', '1.0')
634+
.send({
635+
url: 'http://127.0.0.1:9999/webhook',
636+
taskId: 'task-1',
637+
tenant: '',
638+
})
639+
.expect(201);
640+
641+
const protoResponse = TaskPushNotificationConfig.fromJSON(response.body);
642+
assert.equal(protoResponse.taskId, 'task-1');
643+
assert.equal(protoResponse.id, 'server-assigned-uuid');
644+
645+
// Sanity: the handler received a config with no id, proving REST
646+
// did not pre-reject and did not fabricate an id of its own.
647+
const passedConfig = (mockRequestHandler.createTaskPushNotificationConfig as Mock).mock
648+
.calls[0][0];
649+
assert.equal(passedConfig.id, '');
650+
});
615651
});
616652

617653
describe('GET /tasks/:taskId/pushNotificationConfigs', () => {

test/server/push_notification_store.spec.ts

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -42,15 +42,24 @@ describe('InMemoryPushNotificationStore.load() (canonical, version-agnostic read
4242
expect(loaded).toEqual([]);
4343
});
4444

45-
it('defaults a missing config id to the taskId on save', async () => {
45+
it('assigns a server-side UUID when saved with an empty config id', async () => {
46+
// Spec §3.1.7 / §5.1: id is the *result* of Create, not an input
47+
// requirement. The store assigns a UUID at the save boundary so
48+
// every entry point — `createTaskPushNotificationConfig`,
49+
// `sendMessage`'s and `sendMessageStream`'s
50+
// `params.configuration.taskPushNotificationConfig` paths — gets
51+
// the same auto-assignment.
4652
const context = new ServerCallContext({ requestedVersion: A2A_PROTOCOL_VERSION });
47-
const config = makeConfig({ id: '' });
4853

49-
await store.save('task-id-default', context, config);
50-
const loaded = await store.load('task-id-default', context);
54+
await store.save('task-id-empty', context, makeConfig({ id: '' }));
55+
await store.save('task-id-empty', context, makeConfig({ id: '' }));
56+
const loaded = await store.load('task-id-empty', context);
5157

52-
expect(loaded).toHaveLength(1);
53-
expect(loaded[0].id).toBe('task-id-default');
58+
expect(loaded).toHaveLength(2);
59+
const UUIDV4_RE = /^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i;
60+
expect(loaded[0].id).toMatch(UUIDV4_RE);
61+
expect(loaded[1].id).toMatch(UUIDV4_RE);
62+
expect(loaded[0].id).not.toBe(loaded[1].id);
5463
});
5564

5665
it('delete() matches against the config id', async () => {
Lines changed: 227 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,227 @@
1+
import { describe, it, beforeEach, expect } from 'vitest';
2+
3+
import {
4+
DefaultRequestHandler,
5+
InMemoryPushNotificationStore,
6+
InMemoryTaskStore,
7+
TaskStore,
8+
} from '../../../src/server/index.js';
9+
import {
10+
AgentCard,
11+
Task,
12+
TaskPushNotificationConfig,
13+
TaskState,
14+
} from '../../../src/types/pb/a2a.js';
15+
import { DefaultExecutionEventBusManager } from '../../../src/server/events/execution_event_bus_manager.js';
16+
import { ServerCallContext } from '../../../src/server/context.js';
17+
import { MockAgentExecutor } from '../mocks/agent-executor.mock.js';
18+
19+
/**
20+
* Focused coverage for {@link DefaultRequestHandler.createTaskPushNotificationConfig}
21+
* per spec §3.1.7 ("Created configuration with assigned ID") and §5.1
22+
* (functional equivalence across transports).
23+
*
24+
* The contract verified here:
25+
*
26+
* 1. **Id-less Create** — when `params.id` is empty the handler MUST
27+
* assign a server-side UUID and return the persisted record. Prior
28+
* to this fix the store's `id ||= taskId` fallback collapsed every
29+
* parameter-less Create onto a single row, silently overwriting
30+
* previous configs.
31+
*
32+
* 2. **Multiple id-less Creates** — must produce distinct entries,
33+
* each with its own UUID. Regression guard for the same
34+
* destructive-upsert path.
35+
*
36+
* 3. **Explicit id** — when the caller supplies a non-empty id the
37+
* handler MUST persist under that id and return the stored shape
38+
* (not the input params reference), so the caller observes any
39+
* normalization the store performed.
40+
*
41+
* 4. **List after multi-Create** — `listTaskPushNotificationConfigs`
42+
* returns every entry created above (id-less + explicit), proving
43+
* the records weren't merged.
44+
*
45+
* Mirrors a2a-go's UUIDv7-based store (`a2asrv/push/store.go:45-48`) and
46+
* a2a-python's parameter-less Create handling.
47+
*/
48+
describe('DefaultRequestHandler.createTaskPushNotificationConfig (§3.1.7, §5.1)', () => {
49+
let handler: DefaultRequestHandler;
50+
let taskStore: TaskStore;
51+
let pushNotificationStore: InMemoryPushNotificationStore;
52+
53+
const agentCard: AgentCard = {
54+
name: 'Push Notification Agent',
55+
description: 'Test agent for §3.1.7 / §5.1 push-notification create',
56+
version: '1.0.0',
57+
provider: undefined,
58+
documentationUrl: '',
59+
supportedInterfaces: [
60+
{
61+
url: 'http://localhost/a2a',
62+
protocolBinding: 'HTTP+JSON',
63+
tenant: '',
64+
protocolVersion: '1.0',
65+
},
66+
],
67+
capabilities: {
68+
extensions: [],
69+
streaming: true,
70+
pushNotifications: true,
71+
},
72+
securitySchemes: {},
73+
securityRequirements: [],
74+
defaultInputModes: ['text/plain'],
75+
defaultOutputModes: ['text/plain'],
76+
skills: [],
77+
signatures: [],
78+
};
79+
80+
const serverContext = new ServerCallContext();
81+
82+
beforeEach(async () => {
83+
taskStore = new InMemoryTaskStore();
84+
pushNotificationStore = new InMemoryPushNotificationStore();
85+
handler = new DefaultRequestHandler(
86+
agentCard,
87+
taskStore,
88+
new MockAgentExecutor(),
89+
new DefaultExecutionEventBusManager(),
90+
pushNotificationStore
91+
);
92+
});
93+
94+
const makeTask = (id: string): Task => ({
95+
id,
96+
contextId: `ctx-${id}`,
97+
status: { state: TaskState.TASK_STATE_WORKING, message: undefined, timestamp: undefined },
98+
artifacts: [],
99+
history: [],
100+
metadata: {},
101+
});
102+
103+
const makeIdLessConfig = (taskId: string, url: string): TaskPushNotificationConfig => ({
104+
tenant: '',
105+
taskId,
106+
id: '',
107+
url,
108+
token: '',
109+
authentication: undefined,
110+
});
111+
112+
// §RFC 4122 v4: 8-4-4-4-12 hex digits, version nibble 4. The handler
113+
// uses `uuid.v4`; this matcher catches accidental swaps to other id
114+
// schemes (e.g. taskId fallback) without coupling to library specifics.
115+
const UUIDV4_RE = /^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i;
116+
117+
it('assigns a server-side UUID when params.id is empty and returns the persisted record', async () => {
118+
const taskId = 'task-idless-single';
119+
await taskStore.save(makeTask(taskId), serverContext);
120+
121+
const result = await handler.createTaskPushNotificationConfig(
122+
makeIdLessConfig(taskId, 'https://example.test/webhook-1'),
123+
serverContext
124+
);
125+
126+
expect(result.id).toMatch(UUIDV4_RE);
127+
expect(result.taskId).toBe(taskId);
128+
expect(result.url).toBe('https://example.test/webhook-1');
129+
130+
// Verify persistence: the record returned must be the one in the
131+
// store, not a reflection of the input.
132+
const stored = await pushNotificationStore.load(taskId, serverContext);
133+
expect(stored).toHaveLength(1);
134+
expect(stored[0].id).toBe(result.id);
135+
});
136+
137+
it('produces distinct UUIDs for two id-less Creates (no silent upsert)', async () => {
138+
// Regression guard for the old `id ||= taskId` store fallback: two
139+
// parameter-less Creates used to collapse onto a single row keyed by
140+
// taskId, destroying the first config. They must now coexist with
141+
// distinct server-assigned ids.
142+
const taskId = 'task-idless-multi';
143+
await taskStore.save(makeTask(taskId), serverContext);
144+
145+
const first = await handler.createTaskPushNotificationConfig(
146+
makeIdLessConfig(taskId, 'https://example.test/webhook-A'),
147+
serverContext
148+
);
149+
const second = await handler.createTaskPushNotificationConfig(
150+
makeIdLessConfig(taskId, 'https://example.test/webhook-B'),
151+
serverContext
152+
);
153+
154+
expect(first.id).toMatch(UUIDV4_RE);
155+
expect(second.id).toMatch(UUIDV4_RE);
156+
expect(first.id).not.toBe(second.id);
157+
158+
const stored = await pushNotificationStore.load(taskId, serverContext);
159+
expect(stored).toHaveLength(2);
160+
expect(stored.map((c) => c.id).sort()).toEqual([first.id, second.id].sort());
161+
expect(stored.map((c) => c.url).sort()).toEqual([
162+
'https://example.test/webhook-A',
163+
'https://example.test/webhook-B',
164+
]);
165+
});
166+
167+
it('preserves an explicit id and returns a deep clone (caller mutations cannot reach the store)', async () => {
168+
const taskId = 'task-explicit-id';
169+
await taskStore.save(makeTask(taskId), serverContext);
170+
171+
const params: TaskPushNotificationConfig = {
172+
tenant: '',
173+
taskId,
174+
id: 'caller-chosen-id',
175+
url: 'https://example.test/webhook-explicit',
176+
token: 'shh',
177+
authentication: undefined,
178+
};
179+
const result = await handler.createTaskPushNotificationConfig(params, serverContext);
180+
181+
expect(result.id).toBe('caller-chosen-id');
182+
expect(result.url).toBe('https://example.test/webhook-explicit');
183+
184+
// The returned object must be a deep clone, not the input reference —
185+
// caller-side mutations of the returned value must not reach the
186+
// store's internal entry. Same isolation guarantee `store.load()`
187+
// provides.
188+
expect(result).not.toBe(params);
189+
result.url = 'https://attacker.test/';
190+
const stored = await pushNotificationStore.load(taskId, serverContext);
191+
expect(stored[0].url).toBe('https://example.test/webhook-explicit');
192+
});
193+
194+
it('listTaskPushNotificationConfigs returns every entry after mixed id-less + explicit Creates', async () => {
195+
const taskId = 'task-list-after-multi';
196+
await taskStore.save(makeTask(taskId), serverContext);
197+
198+
const idless1 = await handler.createTaskPushNotificationConfig(
199+
makeIdLessConfig(taskId, 'https://example.test/wh-1'),
200+
serverContext
201+
);
202+
const explicit = await handler.createTaskPushNotificationConfig(
203+
{
204+
tenant: '',
205+
taskId,
206+
id: 'pinned',
207+
url: 'https://example.test/wh-2',
208+
token: '',
209+
authentication: undefined,
210+
},
211+
serverContext
212+
);
213+
const idless2 = await handler.createTaskPushNotificationConfig(
214+
makeIdLessConfig(taskId, 'https://example.test/wh-3'),
215+
serverContext
216+
);
217+
218+
const list = await handler.listTaskPushNotificationConfigs(
219+
{ tenant: '', taskId, pageSize: 0, pageToken: '' },
220+
serverContext
221+
);
222+
223+
expect(list.configs).toHaveLength(3);
224+
const idsSeen = list.configs.map((c) => c.id).sort();
225+
expect(idsSeen).toEqual([idless1.id, explicit.id, idless2.id].sort());
226+
});
227+
});

test/server/rest_transport_handler.spec.ts

Lines changed: 22 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -355,15 +355,32 @@ describe('RestTransportHandler', () => {
355355
expect(result).to.deep.equal(mockConfig);
356356
});
357357

358-
it('should throw InvalidParams if id is missing', async () => {
359-
const invalidConfig = {
358+
it('should accept id-less config and delegate to the handler (handler assigns UUID)', async () => {
359+
// Spec §3.1.7 / §5.1: id is optional across all transports. The
360+
// handler generates a server-side UUID when omitted, so REST must
361+
// not pre-reject id-less requests (previously threw
362+
// `RequestMalformedError('id is required')`, breaking parity
363+
// with JSON-RPC, gRPC, and the reference a2a-python / a2a-go REST
364+
// dispatchers).
365+
const idLessConfig = {
360366
taskId: 'task-1',
361367
url: 'https://example.com/webhook',
362368
};
369+
const assignedConfig = { ...idLessConfig, id: 'server-assigned-uuid' };
370+
(mockRequestHandler.createTaskPushNotificationConfig as Mock).mockResolvedValue(
371+
assignedConfig
372+
);
363373

364-
await expect(
365-
transportHandler.createTaskPushNotificationConfig(invalidConfig as any, mockContext)
366-
).rejects.toThrow('id is required');
374+
const result = await transportHandler.createTaskPushNotificationConfig(
375+
idLessConfig as any,
376+
mockContext
377+
);
378+
379+
expect(mockRequestHandler.createTaskPushNotificationConfig).toHaveBeenCalledWith(
380+
idLessConfig,
381+
mockContext
382+
);
383+
expect(result).to.deep.equal(assignedConfig);
367384
});
368385
});
369386

0 commit comments

Comments
 (0)