KG-508 Update Tool API to fix name and descriptor discrepancy and make it more robust #1226
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
EugeneTheDev
force-pushed
the
eugenethedev/fix-tool
branch
from
c1c2967 to
17f3530
Compare
Qodana for JVM1168 new problems were found
@@ Code coverage @@
+ 72% total lines covered
16835 lines analyzed, 12215 lines covered
# Calculated according to the filters of your coverage tool☁️ View the detailed Qodana report Contact Qodana teamContact us at [email protected]
|
kpavlov
approved these changes
Dec 10, 2025
Contributor
kpavlov
left a comment
kpavlov
left a comment
There was a problem hiding this comment.
Hey @EugeneTheDev, could you please add a PR description and update the identity tool’s name and description before we merge?
Comment on lines
+112
to
+115
| name = SubgraphWithTaskUtils.FINALIZE_SUBGRAPH_TOOL_NAME, | ||
| description = SubgraphWithTaskUtils.FINALIZE_SUBGRAPH_TOOL_DESCRIPTION |
Contributor
There was a problem hiding this comment.
Is it a copy from finalize tool?
Collaborator
Author
There was a problem hiding this comment.
It was like this before. This identityTool is internal and is used actually only as a finalize tool to provide the final result, hence the finalize tool name. Sounds like we can leave it as it is. Or do you want me to make name and description configurable (although there's no use for it currently). wdyt?
EugeneTheDev
force-pushed
the
eugenethedev/fix-tool
branch
from
38399db to
ec28f10
Compare
Collaborator
Author
|
Updated description |
Qodana for JVM1206 new problems were found
@@ Code coverage @@
+ 72% total lines covered
17235 lines analyzed, 12530 lines covered
# Calculated according to the filters of your coverage tool☁️ View the detailed Qodana report Contact Qodana teamContact us at [email protected]
|
Merged
vova-jb
pushed a commit
that referenced
this pull request
Jan 27, 2026
…e it more robust (#1226) **This PR introduces breaking changes in Tool API** Currently `argsSerializer`, `resultSerializer`, `name`, `description` and `descriptor` are meant to be overriden to provide your own values. Since we can also generate `descriptor` automatically now, it introduces a discrepancy between property values and the actual state of things. It's described in [KG-508](https://youtrack.jetbrains.com/issue/KG-508). So current version makes for unstable and somewhat confusing API. In this PR, I've moved these configurable tool properties to the constructors instead. This avoids confusion, since now the ways in which you can configure the tool are visible more explicitly - either provide your own `descriptor`, or opt for an automatic generation and only provide `name` and `description`. Updated API also looks a bit more concise and fluent, IMHO, since you have to write less `override` now. Compare these: ```kt // Before object DummyTool : SimpleTool<Unit>() { override val argsSerializer = Unit.serializer() override val name: String = "dummy" override val description: String = "Dummy tool for testing" override suspend fun doExecute(args: Unit): String = "Dummy result" } // After object DummyTool : SimpleTool<Unit>( argsSerializer = Unit.serializer(), name = "dummy", description = "Dummy tool for testing" ) { override suspend fun execute(args: Unit): String = "Dummy result" } ``` Also, another breaking change, as you have already seen from the example above, is `doExecute` removal. It is not needed anymore, since tools support primitive types now, so you can just use a regular `execute`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR introduces breaking changes in Tool API
Currently
argsSerializer,resultSerializer,name,descriptionanddescriptorare meant to be overriden to provide your own values. Since we can also generatedescriptorautomatically now, it introduces a discrepancy between property values and the actual state of things. It's described in KG-508. So current version makes for unstable and somewhat confusing API.In this PR, I've moved these configurable tool properties to the constructors instead. This avoids confusion, since now the ways in which you can configure the tool are visible more explicitly - either provide your own
descriptor, or opt for an automatic generation and only providenameanddescription. Updated API also looks a bit more concise and fluent, IMHO, since you have to write lessoverridenow. Compare these:Also, another breaking change, as you have already seen from the example above, is
doExecuteremoval. It is not needed anymore, since tools support primitive types now, so you can just use a regularexecute