feat(agents): Introduce wrapper for cli agents#1413
Conversation
44e0012 to
3a801b4
Compare
8504b5b to
a01e00e
Compare
a01e00e to
4e5abf7
Compare
Ololoshechkin
left a comment
There was a problem hiding this comment.
Please, add more tests on structured output, and see my other comments as well.
Additionally, please consider adding Java-friendly builder AIAgent.builder().agentConfig(...).cli(AIAgentCliStrategy. ...) ...
(add cli or cliStrategy in addition to .functionalStrategy, .goapStrategy and .graphStrategy).
-- this part requires both Also add Java test and check how it looks there :) |
Ololoshechkin
left a comment
There was a problem hiding this comment.
also maybe we'd need to run integration tests for:
- all types of CLI agents (codex, claude, ?)
- all types of transport (docker, process)
- all OS: mac, windows, linux
cc @aozherelyeva can you help please 🙏
6562dad to
1f00287
Compare
d4b0699 to
d49ceb0
Compare
139deea to
492c841
Compare
sdubov
left a comment
There was a problem hiding this comment.
@antoniibelyshev, thank you for the fixed comments! I've added several more notes related to the code. Could you please check?
8942b99 to
521ad32
Compare
There was a problem hiding this comment.
@antoniibelyshev , I have added several more question for the review. Could you please take a look.
The biggest concern from my side are:
DummyPromptExecutor. It is quite unclear how will it work when somebody call the prompt executor. Maybe it worth adding a stubs that will delegate execution to the CLI agent in future and just blank for now
I will give an approve from my side to not block the merging, but I think it would be nice if someone from the team look on these changes as well. cc @EugeneTheDev, @Ololoshechkin
Could you please also check the documentation and Module.md file. It might worth add some examples in the md file.
| prompt = agentConfig.prompt, | ||
| model = agentConfig.model, | ||
| responseProcessor = agentConfig.responseProcessor, | ||
| promptExecutor = DummyPromptExecutor, |
There was a problem hiding this comment.
Could you please clarify this logic for me? Am I right that you replace the prompt executor here with a dummy instance and delegate the execution to the CLI process. If this is correct, I would like to clarify several questions:
- Do you expect to change the behaviour in future for better integration and interactions? For example, if I want to send some requests from Koog side inside a running CLI agent or respond to some questions received from CLI Agent?
- How would that work with features? Let's say I add a feature that use prompt executor. Will those calls all throw?
- Could you please add KDoc to the
DummyPromptExecutorwith some details if this is behaviour is not documented anywhere?
…h raw cli and with cli in docker
521ad32 to
cacd812
Compare
EugeneTheDev
left a comment
There was a problem hiding this comment.
Please extract cli package from agents-core intro a separate module. We should avoid putting non-core stuff in core modules as much as possible
| FROM node:20-slim | ||
|
|
||
| RUN apt-get update && apt-get install -y git && rm -rf /var/lib/apt/lists/* | ||
| RUN npm install -g @openai/codex @anthropic-ai/claude-code && npm cache clean --force |
There was a problem hiding this comment.
Please pin the exact version/hash when doing npm install, to prevent supply chain vulenrabilities
| node-version: 20 | ||
|
|
||
| - name: Install CLI agents globally | ||
| run: npm install -g @openai/codex @anthropic-ai/claude-code |
There was a problem hiding this comment.
Why do you run npm install here, if the Dockerfile already has it? Seems like it's not needed here. And again, please always pin dependencies to exact versions/hashes
EugeneTheDev
left a comment
There was a problem hiding this comment.
Please also consider organizing agents cli package a bit. Currently, it has a flat structure, but I guess some of these classes can be grouped into several packages, for better readbility
| /** | ||
| * Base class for cli-agent-related exceptions. | ||
| */ | ||
| public open class CliException(message: String, cause: Throwable? = null) : RuntimeException(message, cause) |
There was a problem hiding this comment.
Why is it a RuntimeException and not a regular Exception?
| /** | ||
| * Interface for cli transport implementations. | ||
| */ | ||
| public expect interface CliTransport { |
There was a problem hiding this comment.
As far as I understand the use-case, this interface doesn't have to be expect/actual. You don't actually use it, the interface on both platforms looks the same. It's the platform-specific implementations that are located in platform-specific source sets anyway and contain platform-specific code. So you can make this one a regular interface
There was a problem hiding this comment.
The reason I made it expect/actual is that I wanted to have CliTransport.default(), CliTransport.withDocker, etc., but these are only available on jvm for now. Does this make sense or can I do it better?
| /** | ||
| * Converts a JSON primitive to a string | ||
| */ | ||
| public val JsonElement.stringVal: String? |
There was a problem hiding this comment.
Putting extension functions/propeties inside objects/classes is usually a bad idea, if they're used outside of that specific scope, because it will be harder to discover and use them. Consider moving these to the top-level. Also, are these internal helpers or an actual public API? If it's the former, consider adding internal api annotation
feat(CliAIAgent):
introduce
CliAIAgentwhich calls another cli agent under the hood: e.g. claude, codex, etc.CliAIAgentis an instance ofAIAgent, and can also be used as a node in a graph, e.g.val claudeNode by ClaudeCodeAgent(...).asNode()Motivation and Context
Breaking Changes
Type of the changes
Checklist
developas the base branchAdditional steps for pull requests adding a new feature