-
Notifications
You must be signed in to change notification settings - Fork 360
feat: refactor agent skills and support OpenAI Agents SDK #1100
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
base: main
Are you sure you want to change the base?
Conversation
d449fe2 to
5b78fd3
Compare
39b1844 to
e1071ed
Compare
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.
Pull request overview
This pull request refactors agent skills architecture and adds OpenAI Agents SDK support by creating a centralized kagent-skills package that both ADK and OpenAI integrations can leverage. The refactoring improves code reusability and maintainability while enabling OpenAI agents to utilize the same skills infrastructure as ADK agents.
Key Changes:
- Created
kagent-skillspackage with core skills logic (discovery, session management, shell operations, prompts) - Added
kagent-openaipackage with OpenAI Agents SDK integration including tools, session service, event conversion, and agent executor - Refactored
kagent-adkto use shared skills package, removing duplicated code
Reviewed changes
Copilot reviewed 50 out of 54 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
python/packages/kagent-skills/ |
New centralized package for skills discovery, file operations, shell execution, and session management |
python/packages/kagent-openai/ |
New OpenAI SDK integration with A2A protocol support, session persistence, and tools |
python/samples/openai/basic_agent/ |
Sample OpenAI agent demonstrating calculator and weather tools |
python/packages/kagent-adk/src/kagent/adk/tools/ |
Refactored to use kagent-skills package instead of local implementations |
python/packages/kagent-crewai/ |
Import order fixes and CrewAI version update to >= 1.2.0 |
python/Dockerfile |
Added kagent-skills package, updated PYTHONPATH and venv configuration |
go/test/e2e/ |
Added e2e tests for OpenAI agent invocation with mocked LLM responses |
python/pyproject.toml |
Added samples/openai to workspace members |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| bash_venv_path = os.environ.get("BASH_VENV_PATH") | ||
| if bash_venv_path: | ||
| bash_venv_bin = os.path.join(bash_venv_path, "bin") | ||
| # Prepend bash venv to PATH so its python and pip are used | ||
| env["PATH"] = f"{bash_venv_bin}:{env.get('PATH', '')}" | ||
| env["VIRTUAL_ENV"] = bash_venv_path |
Copilot
AI
Dec 4, 2025
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.
Missing error handling for the case when BASH_VENV_PATH environment variable is set but the path doesn't exist or is invalid. If bash_venv_path is set but the directory doesn't exist, this could lead to the command failing with unclear errors.
Consider adding validation:
if bash_venv_path:
if not os.path.exists(bash_venv_path):
logger.warning(f"BASH_VENV_PATH {bash_venv_path} does not exist, using system python")
else:
bash_venv_bin = os.path.join(bash_venv_path, "bin")
env["PATH"] = f"{bash_venv_bin}:{env.get('PATH', '')}"
env["VIRTUAL_ENV"] = bash_venv_path| timeout=self._config.execution_timeout, | ||
| ) | ||
|
|
||
| except TimeoutError: |
Copilot
AI
Dec 4, 2025
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.
The TimeoutError exception should be asyncio.TimeoutError for clarity and consistency. While they may be the same in Python 3.13+, being explicit about using asyncio's TimeoutError improves code clarity, especially since asyncio.wait_for is being used above.
Change line 284 from:
except TimeoutError:to:
except asyncio.TimeoutError:| except TimeoutError: | |
| except asyncio.TimeoutError: |
| "SkillsTool", | ||
| "SkillsPlugin", | ||
| "SkillsToolset", | ||
| "generate_shell_skills_system_prompt", |
Copilot
AI
Dec 4, 2025
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.
The name 'generate_shell_skills_system_prompt' is exported by all but is not defined.
29b2fea to
32bcdf1
Compare
Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io> Co-authored-by: Eitan Yarmush <eitan.yarmush@solo.io> Co-authored-by: Jet Chiang <jetjiang.ez@gmail.com>
Signed-off-by: Jet Chiang <jetjiang.ez@gmail.com>
Signed-off-by: Jet Chiang <jetjiang.ez@gmail.com>
Signed-off-by: Jet Chiang <jetjiang.ez@gmail.com>
Signed-off-by: Jet Chiang <jetjiang.ez@gmail.com>
Signed-off-by: Jet Chiang <jetjiang.ez@gmail.com>
Signed-off-by: Jet Chiang <jetjiang.ez@gmail.com>
Signed-off-by: Jet Chiang <jetjiang.ez@gmail.com>
Signed-off-by: Jet Chiang <jetjiang.ez@gmail.com>
Signed-off-by: Jet Chiang <jetjiang.ez@gmail.com>
Signed-off-by: Jet Chiang <jetjiang.ez@gmail.com>
Signed-off-by: Jet Chiang <jetjiang.ez@gmail.com>
Signed-off-by: Jet Chiang <jetjiang.ez@gmail.com>
Signed-off-by: Jet Chiang <jetjiang.ez@gmail.com>
Signed-off-by: Jet Chiang <jetjiang.ez@gmail.com>
Signed-off-by: Jet Chiang <jetjiang.ez@gmail.com>
Signed-off-by: Jet Chiang <jetjiang.ez@gmail.com>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
32bcdf1 to
731e51e
Compare
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 used this stub as a temporary solution to fix opentelemetry-instrumentation-openai-agents due to some dependency hell. Ideally we would update it to >0.50.0 since the traceloop import is made optional there, but it conflicts with the opentelemtry api pinned by ADK.
If needed I can look for a more elegant method to fix the issue, but I tried different versions of traceloop and it has some weird version pinning with the instrumentation library and some versions have bugs that just crashed... 😭
Completes #1068
kagent-skills, update ADK and OpenAI SDK to wrap them with their functions tool implementations