Skip to content

Conversation

Ololoshechkin
Copy link
Collaborator

Thank you for opening a pull request! Please add a brief description of the proposed change here.

Also, please tick the appropriate points in the checklist below.


Type of the change

  • New feature
  • Bug fix
  • Documentation fix
  • Tests improvement

Checklist for all pull requests

  • The pull request has a description of the proposed change
  • I read the Contributing Guidelines before opening the pull request
  • The pull request uses develop as the base branch
  • Tests for the changes have been added
  • All new and existing tests passed
Additional steps for pull requests adding a new feature
  • An issue describing the proposed change exists
  • The pull request includes a link to the issue
  • The change was discussed and approved in the issue
  • Docs have been added / updated

@Ololoshechkin Ololoshechkin requested review from e5l and Rizzen July 11, 2025 11:34
@Ololoshechkin
Copy link
Collaborator Author

Note: this is a draft proposal written in examples. Design should be improved and Ktor plugin should be moved as a separate module available in public API if we decide it's worth doing.

cc: @e5l WDYT? Can we add this into Ktor marketplace at some point?

@Ololoshechkin Ololoshechkin changed the title [Draft, Proposal] Ktor integration example via Ktor plugins (proposal) [Draft, Proposal] Ktor integration via Ktor plugins (proposal) Jul 12, 2025
@Ololoshechkin Ololoshechkin force-pushed the vbr/ktor-integration branch from 6de7da0 to a08ba11 Compare July 13, 2025 02:11
@Ololoshechkin Ololoshechkin changed the title [Draft, Proposal] Ktor integration via Ktor plugins (proposal) [Proposal] Add Ktor integration via Ktor plugin Jul 13, 2025
@Ololoshechkin Ololoshechkin requested a review from nomisRev July 13, 2025 02:12
@Ololoshechkin
Copy link
Collaborator Author

@nomisRev , I've added configuration from yaml/conf file + overriding from DSL.

Also created a separate module with the actual plugin.

@Ololoshechkin Ololoshechkin changed the title [Proposal] Add Ktor integration via Ktor plugin [Proposal] Add Ktor integration via Koog ktor plugin Jul 13, 2025
Copy link

github-actions bot commented Jul 13, 2025

Qodana for JVM

369 new problems were found

Inspection name Severity Problems
Check Kotlin and Java source code coverage 🔶 Warning 352
Unused import directive 🔶 Warning 13
Vulnerable imported dependency 🔶 Warning 4
@@ Code coverage @@
+ 67% total lines covered
8344 lines analyzed, 5661 lines covered
# Calculated according to the filters of your coverage tool

☁️ View the detailed Qodana report

Contact Qodana team

Contact us at [email protected]

@Ololoshechkin Ololoshechkin requested a review from devcrocod July 13, 2025 03:36
Copy link
Collaborator

@kpavlov kpavlov left a comment

Choose a reason for hiding this comment

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

Thank you, @Ololoshechkin — these are some great features. Would you be open to splitting the Ktor and Spring Boot integrations into separate PRs, since they represent distinct functionality? Also, do you think it might make sense to refactor the configuration class structure to improve extensibility, and possibly delegate model name mappings to the respective LLM providers?

}
}

private val OPENAI_MODELS_MAP = mapOf(
Copy link
Collaborator

Choose a reason for hiding this comment

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

This and others: Let's move it to OpenAI module. Such mappings should live in the provider modules and have no relation to KTor

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this definitely needs to be moved but it feels quite impactful to the core core types so I propose to review the code and propose a solution to move this to core in a separate PR to not complicate things further in this PR.

@Ololoshechkin
Copy link
Collaborator Author

@kpavlov Hi! Thanks!
Speing Boot was already added -- I only generated KDocs with AI Assistant as they were missing :)

I only introduced Ktor.

But actually you are right -- I should open a PR with kdocs separately and include in the 0.3.0 release

@kpavlov
Copy link
Collaborator

kpavlov commented Jul 13, 2025

KDoc is not critical, the config and test structure is more important. Separate PR to refactor this is ok, if you promise to refactor ;)
Would it make sense to include Ktor support in 0.4 and have some time to polish and test it up and down?

@Ololoshechkin
Copy link
Collaborator Author

@kpavlov yes, I think it can go to 0.4.0, definitely not 0.3.0 :)

So this PR is rather a draft that someone would finish while I'm on vacation))

@e5l
Copy link
Member

e5l commented Jul 14, 2025

@Ololoshechkin, sure. It can be done through the https://github.com/ktorio/ktor-plugin-registry without release

Copy link
Member

@e5l e5l left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@nomisRev nomisRev left a comment

Choose a reason for hiding this comment

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

Awesome work 👏 👏 👏
I have some critical concerns about some of the API, but I think we can solve this in a subsequent PR as well so I'm not going to block this PR contains the important foundation for the Ktor Koog plugin!

Please let me know in my comments what you think 🙏

@nomisRev
Copy link
Contributor

@Ololoshechkin I can take over if you want 👍

@ptitjes
Copy link
Contributor

ptitjes commented Jul 14, 2025

@Ololoshechkin @nomisRev can we directly put this in an integrations/integrations-ktor/integrations-ktor-plugin?

There is already the Spring integration that is standalone in the the root of the project. I also want to introduce a Redis VectorStore, which will require to move up the current Redis PromptCache in order to introduce an integrations/integrations-redis/integrations-redis-shared module to share the configuration and connection logic.

I am not sure if integrations is the right word, but I think it is rather necessary to regroup the integrations and not accumulate modules at the root of the project. WDYT?

@nomisRev
Copy link
Contributor

nomisRev commented Jul 14, 2025

I am not sure if integrations is the right word, but I think it is rather necessary to regroup the integrations and not accumulate modules at the root of the project. WDYT?

Great point! Personally I have no strong opinion on how to organise this. The projects I'm invoked with follow similar strategies but this project seems to have different needs. "integrations/integrations-redis/integrations-redis-shared" could be a good strategy, as in the future we might want to split up the Ktor support into separate modules as well.

@Ololoshechkin Ololoshechkin changed the title [Proposal] Add Ktor integration via Koog ktor plugin Add Ktor integration via Koog ktor plugin Jul 20, 2025
ariawisp added a commit to ariawisp/koog that referenced this pull request Jul 26, 2025
nomisRev added 2 commits July 27, 2025 14:14
…build files, tests, and code for consistency. Rewrite AI agent functions with improved parameterization and modularity.
@nomisRev nomisRev force-pushed the vbr/ktor-integration branch from a8fe1a0 to 2282684 Compare July 27, 2025 12:15
nomisRev added 3 commits July 27, 2025 14:28
…Ktor library definitions in `libs.versions.toml`.
…pe to run the configuration on until we have suspend plugins in Ktor 4.0
…cope to run the configuration on until we have suspend plugins in Ktor 4.0

- Protect toolRegistry with Mutex
Copy link
Contributor

@devcrocod devcrocod left a comment

Choose a reason for hiding this comment

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

LGTM!
I have a few questions and comments, please take a look

nomisRev added 4 commits July 29, 2025 15:17
…d scripts accordingly."

This reverts commit b8d3ebc.

# Conflicts:
#	settings.gradle.kts
Revert Ktor Version Catalog Android Example
@nomisRev
Copy link
Contributor

@devcrocod I removed all duplicated default values from the Ktor plugin, and delegate to the defaults from core.

I propose to remove the default model, and explicitly require the model when creating an agent. This additional argument will result in a nicer, and type safer API. This would also remove the need for (String) -> LLMModel, and then we can re-evaluate later if we really need it.

WDYT?

nomisRev added 3 commits July 29, 2025 20:45
…configuration structure; remove `defaultLLM` and `agentTools` for better encapsulation.
…factor `aiAgent` to support `suspend`, and encapsulate `scope` in `AgentConfig` for safer concurrency.
…ngs, and ensure explicit `model` parameter usage in AI agent functions.
anthropic.apikey: "your-anthropic-api-key"
google.apikey: "your-google-api-key"
openrouter.apikey: "your-openrouter-api-key"
ollama.enable: true
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should require a url for Ollama?

Copy link
Contributor

Choose a reason for hiding this comment

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

It follows the same behavior as the Ollama Koog integration which is the default URL, or ollama.baseUrl = "...", or ollama { baseUrl = "..." } in code.

@devcrocod
Copy link
Contributor

@nomisRev

I propose to remove the default model, and explicitly require the model when creating an agent. This additional argument will result in a nicer, and type safer API.

In my opinion, that’s a reasonable solution

@nomisRev nomisRev merged commit 22c37c4 into develop Jul 30, 2025
7 checks passed
@nomisRev nomisRev deleted the vbr/ktor-integration branch July 30, 2025 11:26
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.

8 participants