Skip to content

fix: send notifications on message not streaming #219

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
holtskinner merged 10 commits into from
Jun 26, 2025
Merged

fix: send notifications on message not streaming #219

holtskinner merged 10 commits into from
Jun 26, 2025

Conversation

@njbrake
Copy link
Contributor

@njbrake njbrake commented Jun 19, 2025 •
edited
Loading

Description

The proposed fix, if the team does want push notifications to be supported in a non-streaming setup

Thank you for opening a Pull Request!
Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Follow the CONTRIBUTING Guide.
  • Make your Pull Request title in the https://www.conventionalcommits.org/ specification.
    • Important Prefixes for release-please:
      • fix: which represents bug fixes, and correlates to a SemVer patch.
      • feat: represents a new feature, and correlates to a SemVer minor.
      • feat!:, or fix!:, refactor!:, etc., which represent a breaking change (indicated by the !) and will result in a SemVer major.
  • Ensure the tests and linter pass (Run nox -s format from the repository root to format)
  • Appropriate docs were updated (if necessary)

Fixes #218

@njbrake njbrake requested a review from a team as a code owner June 19, 2025 13:20
@njbrake njbrake requested a review from mikeas1 June 19, 2025 13:20
@holtskinner holtskinner self-requested a review June 20, 2025 14:52
holtskinner
holtskinner requested changes Jun 20, 2025
View reviewed changes
Copy link
Member

@holtskinner holtskinner left a comment

Choose a reason for hiding this comment

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

@njbrake Be sure to fix the tests to address this:

https://github.com/google-a2a/a2a-python/actions/runs/15758886077/job/44420533459?pr=219

@swapydapy
Copy link
Contributor

swapydapy commented Jun 20, 2025

Good catch, probably this got lost during code refactors.
Please also add a testcase to verify the push notifs being sent: https://github.com/google-a2a/a2a-python/blob/main/tests/server/request_handlers/test_default_request_handler.py

swapydapy
swapydapy requested changes Jun 20, 2025
View reviewed changes
@njbrake
Copy link
Contributor Author

njbrake commented Jun 20, 2025

@swapydapy will do! Wanted to get the initial thumbs up before I went in too deep 😂 I'll fix the unit tests, add a new one, and also see if there are any opportunities to consolidate logic between the send_message and send_message_streaming methods.

@njbrake njbrake requested review from holtskinner and swapydapy June 23, 2025 09:53
@holtskinner holtskinner force-pushed the brake/push_notification branch from 3cc7dc7 to 6a02d35 Compare June 23, 2025 20:21
@holtskinner holtskinner requested a review from a team as a code owner June 23, 2025 20:21
@holtskinner
Copy link
Member

holtskinner commented Jun 26, 2025

@njbrake Can you resolve the merge conflict?

@njbrake
Copy link
Contributor Author

njbrake commented Jun 26, 2025

@holtskinner done. Is there something I can do to help support this being merged? Not sure if the delay is because of my consolidation of some of the logic between on_message_send and on_message_send_streaming, I can remove that cleanup if you're uncomfortable with the changes.

@holtskinner
Copy link
Member

holtskinner commented Jun 26, 2025

@holtskinner done. Is there something I can do to help support this being merged? Not sure if the delay is because of my consolidation of some of the logic between on_message_send and on_message_send_streaming, I can remove that cleanup if you're uncomfortable with the changes.

No, the rest of the A2A Dev Team just haven't had time to review yet.

@holtskinner holtskinner merged commit 91539d6 into a2aproject:main Jun 26, 2025
5 checks passed
@release-please release-please bot mentioned this pull request Jun 26, 2025
Merged
@njbrake njbrake deleted the brake/push_notification branch June 26, 2025 13:18
Paulchen5 pushed a commit to Paulchen5/a2a-python that referenced this pull request Jun 30, 2025
@njbrake @Paulchen5
fix: send notifications on message not streaming (a2aproject#219)
f8f536e 
# Description

The proposed fix, if the team does want push notifications to be
supported in a non-streaming setup

Fixes a2aproject#218
holtskinner pushed a commit that referenced this pull request Jun 30, 2025
@njbrake @holtskinner
fix: send notifications on message not streaming (#219)
974e69f 
# Description

The proposed fix, if the team does want push notifications to be
supported in a non-streaming setup

Fixes #218
holtskinner added a commit that referenced this pull request Jun 30, 2025
@release-please @holtskinner
chore(main): release 0.2.10 (#202)
61df7d0 
🤖 I have created a release *beep* *boop*
---


##
[0.2.10](v0.2.8...v0.2.10)
(2025-06-30)


### ⚠ BREAKING CHANGES

* Update to A2A Spec Version
[0.2.5](https://github.com/a2aproject/A2A/releases/tag/v0.2.5)
([#197](#197))

### Features

* Add `append` and `last_chunk` to `add_artifact` method on
`TaskUpdater`
([#186](#186))
([8c6560f](8c6560f))
* add a2a routes to existing app
([#188](#188))
([32fecc7](32fecc7))
* Add middleware to the client SDK
([#171](#171))
([efaabd3](efaabd3))
* Add more task state management methods to TaskUpdater
([#208](#208))
([2b3bf6d](2b3bf6d))
* raise error for tasks in terminal states
([#215](#215))
([a0bf13b](a0bf13b))

### Bug Fixes

* `consume_all` doesn't catch `asyncio.TimeoutError` in python 3.10
([#216](#216))
([39307f1](39307f1))
* Append metadata and context id when processing TaskStatusUpdateE…
([#238](#238))
([e106020](e106020))
* Fix reference to `grpc.aio.ServicerContext`
([#237](#237))
([0c1987b](0c1987b))
* Fixes Short Circuit clause for context ID
([#236](#236))
([a5509e6](a5509e6))
* Resolve `APIKeySecurityScheme` parsing failed
([#226](#226))
([aa63b98](aa63b98))
* send notifications on message not streaming
([#219](#219))
([91539d6](91539d6)),
closes [#218](#218)

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

---------

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Co-authored-by: Holt Skinner <[email protected]>
@a2a-bot a2a-bot mentioned this pull request Nov 19, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@holtskinner holtskinner holtskinner approved these changes

@mikeas1 mikeas1 mikeas1 approved these changes

@swapydapy swapydapy Awaiting requested review from swapydapy

Assignees

@swapydapy swapydapy

@mikeas1 mikeas1

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug]: Push Notifications Only sent in on_message_send_stream, not sent in on_message_send

4 participants

@njbrake @swapydapy @holtskinner @mikeas1