-
Notifications
You must be signed in to change notification settings - Fork 310
[KG-4] Initial version of A2A SDK and integration #875
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
Conversation
9128245 to
ea01449
Compare
kpavlov
left a comment
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.
just notes
| * Retrieves [AgentCard] by calling [AgentCardResolver.resolve]. | ||
| * Saves it to the cache. | ||
| */ | ||
| public open suspend fun getAgentCard(): AgentCard { |
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.
nit: As a User, I would expect this method to use cache transparently.
Maybe use cache TTL and/or introduce refreshAgentCard()?
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.
Regular getAgentCard already refreshes the cache, because it uses resoslver every time. And so is get authenticated card. cached card is a convenience method to get local version of the card without fetching it
| request: Request<Nothing?>, | ||
| ctx: ClientCallContext = ClientCallContext.Default | ||
| ): Response<AgentCard> { | ||
| check(getAgentCard().supportsAuthenticatedExtendedCard == true) { |
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.
this method bypasses cache
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.
Thanks
| request: Request<MessageSendParams>, | ||
| ctx: ClientCallContext = ClientCallContext.Default | ||
| ): Response<CommunicationEvent> { | ||
| return transport.sendMessage(request, ctx) |
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.
future improvement: check supported features and fail when operation is not supported
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 already do this, e.g. streaming and push notifications
| contentType(ContentType.Application.Json) | ||
| } | ||
|
|
||
| install(ContentNegotiation) { |
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.
nit: Needs logging also
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.
Logging is definitely needed everywhere in A2A, will add this separately later
| @@ -0,0 +1,476 @@ | |||
| package ai.koog.a2a.model | |||
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.
nit: annotations, consts, and utils packages are plural, model and transport are single. Maybe rename for consistency?
| @JvmInline | ||
| @Serializable | ||
| public value class TransportProtocol(public val value: String) { | ||
| @Suppress("MissingKDocForPublicAPI") |
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.
maybe, let's generate the KDoc
910e9b2 to
cefa010
Compare
Ololoshechkin
left a comment
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.
🗿
041444c to
438ef9e
Compare
| ctx: ClientCallContext = ClientCallContext.Default | ||
| ): Response<AgentCard> { | ||
| check(getAgentCard().supportsAuthenticatedExtendedCard == true) { | ||
| check(cachedAgentCard().supportsAuthenticatedExtendedCard == true) { |
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
Qodana for JVM1003 new problems were found
@@ Code coverage @@
+ 71% total lines covered
14234 lines analyzed, 10107 lines covered
# Calculated according to the filters of your coverage tool☁️ View the detailed Qodana report Contact Qodana teamContact us at [email protected]
|
35611e4 to
2b9f211
Compare
…dencies in build.gradle.kts
2b9f211 to
1a80d6c
Compare
Introduce initial version of A2A SDK and Koog agent A2A integration: