-
Notifications
You must be signed in to change notification settings - Fork 162
Add new a2a
call task to the specification and schema
#1114
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.
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
base: main
Are you sure you want to change the base?
Add new a2a
call task to the specification and schema
#1114
Conversation
Signed-off-by: Emmanuel Allen <[email protected]>
@ricardozanini @cdavernas Our first PR which covers some of our discussions about A2A lol , please review and if necessary, we can even discuss further on our Thursday meeting :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many thanks!! Just a few comments so we can start the conversation.
| Name | Type | Required | Description| | ||
|:--|:---:|:---:|:---| | ||
| method | `string` | `yes` | The A2A JSON-RPC method to send. | | ||
| agentCard | [`externalResource`](#external-resource) | `yes` | The AgentCard resource that describes the agent to call. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this one should be required. I remember in our discussions @cdavernas mentioning that the vcards are not required between the a2a communication.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ricardozanini it's actually needed, as was pointed out in the weekly, to resolve potential authentication.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But is potential authentication required? I'm inquiring because I'm integrating with a few agents and in dev, it's not. I don't think this field should be required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It shouldn't be required, indeed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's not required we will need a separate server field where one of them is required. Because if we don't the workflow runtime won't know where to send the request.
Something like this:
Name | Type | Required | Description |
---|---|---|---|
agentCard | externalResource |
no |
The AgentCard resource that describes the agent to call. Required if server has not been set. |
server | string |endpoint |
no |
An URI or an object that describes the A2A server to call. Required if agentCard has not been set. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, If I am not mistaken lol, I remember @cdavernas mentioned a server field back to issue #1102 to specify the target server.
A server field specifying the target A2A server,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for the server field. And please clarify that:
server
oragentCard
are exclusively requiredserver
overrides the server address in theagentCard
dsl-reference.md
Outdated
> The `security` and `securitySchemes` fields of the AgentCard contain authentication requirements and schemes for when communicating with the agent. | ||
|
||
> [!NOTE] | ||
> On success the output is the JSON-RPC result. On failure runtimes must raise an error. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have error definitions. I believe we should indicate which errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ricardozanini Totally agree. I think a runtime error should be used, and should carry/document the A2A error code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cdavernas So you think that it should be an error of type https://serverlessworkflow.io/spec/1.0.0/errors/runtime. Where do you think the A2A error code should go because I don't think it should be the status code since they are in the range -32700 to -32000?
An example error conversion for a task not found a2a error could be:
type: https://serverlessworkflow.io/spec/1.0.0/errors/runtime
status: 500
title: Task not found, A2A Code: -32001
description: The specified task id does not correspond to an existing or active task. It might be invalid, expired, or already completed and purged.
instance: /do/0/callA2A
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@emmanuelallen we could put it as status code, because even if it is logically related to http status codes, it is not restricted to them.
I however think your proposal is cleaner. The title could be the code and or a high level description like you proposed, that decision is purely esthetic and should therefore be up to runtimes.
dsl-reference.md
Outdated
|:--|:---:|:---:|:---| | ||
| method | `string` | `yes` | The A2A JSON-RPC method to send. | | ||
| agentCard | [`externalResource`](#external-resource) | `yes` | The AgentCard resource that describes the agent to call. | | ||
| parameters | `any` | `no` | The parameters for the A2A rpc method. For the `message/send` and `message/stream` methods the parameters `message.messageId` and `message.role` must be automatically set if missing. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of saying that runtimes must set role if not specified, I'd make it default to user
.
dsl-reference.md
Outdated
> The `security` and `securitySchemes` fields of the AgentCard contain authentication requirements and schemes for when communicating with the agent. | ||
|
||
> [!NOTE] | ||
> On success the output is the JSON-RPC result. On failure runtimes must raise an error. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ricardozanini Totally agree. I think a runtime error should be used, and should carry/document the A2A error code.
Signed-off-by: Emmanuel Allen <[email protected]>
Please specify parts of this PR update:
Discussion or Issue link:
#1102
What this PR does:
Adds A2A call task to the specification and schema
Additional information:
Push Notifications
A2A push notification currently only support webhooks as a way to send task update notifications. That said external services can convert those webhook invocations into cloud events that the workflow can listen for. Hopefully at some point A2A can support natively or through an extension sending out push notifications as cloud events directly.
Streaming
I am suggesting streaming be handled by collecting up all the objects until the stream is complete and returning them as an ordered array. In the future we can add a
foreach
argument similar to AsyncAPI that when set will run a set of tasks for each object received.