Skip to content

fix: Improve task_push_notification_config presence check.#1047

Merged
bartek-w merged 1 commit intomainfrom
bartekw-empty-push
May 6, 2026
Merged

fix: Improve task_push_notification_config presence check.#1047
bartek-w merged 1 commit intomainfrom
bartekw-empty-push

Conversation

@bartek-w
Copy link
Copy Markdown
Collaborator

@bartek-w bartek-w commented May 5, 2026

Call _push_config_store.set_info only when task_push_notification_config is set.

Due to proto3 semantic bool(params.configuration.task_push_notification_config) was always True.

Fixes #1045 🦕

@bartek-w bartek-w changed the title Fix: Improve task_push_notification_config presence check. fix: Improve task_push_notification_config presence check. May 5, 2026
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the logic in both the default and V2 request handlers to use the Protobuf HasField method for checking the presence of task_push_notification_config. This change ensures that default, empty configurations do not incorrectly trigger push notification setup. A new test case was added to verify this behavior; however, the reviewer noted that the test name is misleading because it asserts that the method is not called, and suggested renaming it for clarity.

Comment thread tests/server/request_handlers/test_default_request_handler_v2.py Outdated
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 5, 2026

🧪 Code Coverage (vs main)

⬇️ Download Full Report

Base PR Delta
src/a2a/server/events/event_queue_v2.py 91.19% 91.71% 🟢 +0.52%
src/a2a/utils/telemetry.py 90.63% 91.41% 🟢 +0.78%
Total 93.01% 93.03% 🟢 +0.02%

Generated by coverage-comment.yml

@bartek-w bartek-w force-pushed the bartekw-empty-push branch from d6278a9 to 72c5395 Compare May 5, 2026 10:17
@bartek-w bartek-w requested a review from ishymko May 5, 2026 10:25
@bartek-w bartek-w marked this pull request as ready for review May 5, 2026 10:25
@bartek-w bartek-w requested a review from a team as a code owner May 5, 2026 10:25
@bartek-w bartek-w force-pushed the bartekw-empty-push branch from 72c5395 to f373093 Compare May 6, 2026 09:02
@bartek-w bartek-w merged commit 94ad594 into main May 6, 2026
38 checks passed
@bartek-w bartek-w deleted the bartekw-empty-push branch May 6, 2026 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DefaultRequestHandlerV2._setup_active_task registers empty push subscriptions due to proto3 truthiness check

2 participants