fix: cap push notification configs per task - #1181
Conversation
🧪 Code Coverage (vs
|
| Base | PR | Delta | |
|---|---|---|---|
| src/a2a/server/events/event_queue_v2.py | 91.79% | 91.28% | 🔴 -0.51% |
| src/a2a/utils/telemetry.py | 91.47% | 90.70% | 🔴 -0.78% |
| Total | 93.00% | 92.98% | 🔴 -0.02% |
Generated by coverage-comment.yml
mykytanetipa
left a comment
mykytanetipa
left a comment
There was a problem hiding this comment.
The push notifications configs per task cap is a reasonable idea, but it's a public-facing behavioral change, so it should be reviewed and accepted by the A2A spec maintainers first. Please open a spec proposal PR at https://github.com/a2aproject/A2A before implementing it in the SDK.
| """ | ||
|
|
||
|
|
||
| class InMemoryPushNotificationConfigStore(PushNotificationConfigStore): |
There was a problem hiding this comment.
The push notification cap is only present in InMemoryPushNotificationConfigStore
DatabasePushNotificationConfigStore and any custom PushNotificationConfigStore subclasses have no cap. I think that limit should belong at the handler layer, not in one store implementation.
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| MAX_PUSH_NOTIFICATION_CONFIGS_PER_TASK = 50 |
There was a problem hiding this comment.
I'd avoid setting MAX_PUSH_NOTIFICATION_CONFIGS_PER_TASK just as a global constant. If this change lands it should be configurable, opt-in, with None default and set by some server config
2 participants
No description provided.