Qodana for JVM

852 new problems were found

@@ Code coverage @@
+ 67% total lines covered
11264 lines analyzed, 7548 lines covered
# Calculated according to the filters of your coverage tool

☁️ View the detailed Qodana report

@sdubov @Ololoshechkin @EugeneTheDev This PR still in progress and I would like to finish it when I'll back. But it's mostly ready implementation-wise, so it's already up to top-level review (GraphAgent/Non-Graph agent separation, pipeline separation, environment separation, etc).
What's remaining - tests, features migration (at least Memory, MessageTokenizer, maybe some more), docs.

@sdubov I don't really like the idea of "universal registration" you suggest.

The idea behind the current design is clear separation of concerns. When you develop a feature for either a graph or non-graph agent, you only deal with the relevant handlers. This keeps the mental model simple — you know exactly which events your feature can react to.

With a universal interface, you end up in situations like: “Okay, I have something called afterNode, let's try it out”, then you install the feature on a non-graph agent and... nothing happens. After some time digging through source code, you finally discover that some handlers in the interface you implemented are silently ignored because of an internal flag in the pipeline.
That kind of invisible behavior would be very frustrating to users — it's unpredictable and hard to debug.

This also opens the door to other confusing scenarios. For example, it would technically allow installing something like the Persistency feature into a non-graph agent. It would appear to install successfully but would silently do nothing, which is extremely unclear for users and could easily lead to subtle bugs.

From an implementation perspective, this would also require the non-graph agent to maintain a graph-like pipeline to satisfy all those extra handlers, even if they’re never actually used. That adds unnecessary complexity and coupling in places where it doesn’t belong.

The current design avoids these problems by using clear, explicit interfaces:

If you’re writing a NonGraphFeature, you only see the relevant events, and everything you implement will always fire.

The type system enforces compatibility — you simply cannot install a non-graph feature into a graph agent or vice versa. This makes incorrect usage impossible at compile time, rather than a silent runtime no-op.

This makes the system much more predictable and maintainable. A developer always knows exactly what events are available, what will trigger, and what won’t. It also keeps the boundaries between graph and non-graph agents crisp, without forcing one to emulate the internals of the other just to support a universal abstraction. Yes, it requires to impement two installs in case of features that should work on both types of agents, but it's lesser price for clear interfaces and type safety.

@Rizzen, I fully agree with your points and concerns. I think there are pros and cons in both approaches. It's up to you, for sure, to estimate which one would be better for an end-user perspective.

Let me clarify some points from my suggestion (as I found that I miss-format the original message) and I have a feeling that the idea might be not clear from the initial description:

It would appear to install successfully but would silently do nothing

I would not expect the feature is fully ignored. My suggestion was to ignore a graph-specific events only, e.g. if you have Tracing feature and you install it in a graph-agent you get trace:

> agent start
> node call llm 1 start
> llm call 1 start
> llm call 1 finish
> node call llm 1 finish
> agent finished

If you install the same feature inside a non-graph-agent, you should get everything except graph-specific events (which are not fired in a non-graph mode) as expected:

> agent start
> llm call 1 start
> llm call 1 finish
> agent finished

I think it might be quite clear for a user that everything graph-specific will not work in non-graph-agent scenarios (it is actually already works the same way with your approach, but you explicitly need to write extra code in the feature for that).

In the approach that you use, you make every feature development more complicated. You need to be familiar with non-graph option in koog. Otherwise, it also might be a surprise, when you cannot install an existing feature when you run non-graph agent. A user need to be aware that they should implement both interfaces to make sure feature works for both of them, and it is not obvious from my vision.

I think the type system should indicate whether a feature was developed with an awareness of non-graph and/or graph scenarios. If there is a lot of overlap between them, then the overlap could be another candidate for indicating an awareness of via type system.

I reckon it probably makes sense to have them separate before identifying that overlap, unless it's very clear already

@sdubov Thank you for clarification, I believe I got your suggestion right.

> agent start
> node call llm 1 start
> llm call 1 start
> llm call 1 finish
> node call llm 1 finish
> agent finished

and

> agent start
> node call llm 1 start
> llm call 1 start
> llm call 1 finish
> node call llm 1 finish
> agent finished

examples are exactly what I meant by "silently ignoring" events. In case of tracing feature it might be still look pretty logical, because it still traces most events for both agents. But with Persistency, for example, it would be just working feature in the case of graph agent and completely no-op feature in the case of non-graph, while still allowing to install it to non-graph agent.

So when we were designing that feature - we were aiming to clearly separate Graph and Non-Graph agents - we have different contexts, different pipelines and different features, preventing to install wrong features to wrong agents on compilation level to avoid strange behaviour and confusion from users.

From the feature developer perspective - if you're one, you are aware of concepts like graph agent, non-graph agent and even graph elements like nodes etc. We can do that with additional documentation on feature development.

Just a note to say that the current persistency feature stores not only the current graph node but also the history and the agent state. I would hope that the persistency feature would allow for explicit checkpoint creation/recall from a programatic agent. If it's necessary to decouple the addition of the current graph node to the checkpoint, through an extension point, then it's the way to go IMO.

Let's make follow-up cosmetic changes in the future PRs :)
Conceptually, this one looks good.

Qodana for JVM

853 new problems were found

@@ Code coverage @@
+ 66% total lines covered
11276 lines analyzed, 7544 lines covered
# Calculated according to the filters of your coverage tool

☁️ View the detailed Qodana report